-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[logos] Support multi-line block arguments in %orig
calls
#114
[logos] Support multi-line block arguments in %orig
calls
#114
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally had time to go over this and understand it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a ton for putting this together! I reviewed this a little while back and all looked good. Can't say anything stands out upon second glance. Thanks for adding the check for Logos within %orig
. Totally agree that a test suite is in order. It's on the list!
Edit: we'll also probably want to update https://theos.dev/docs/logos-syntax to note that block syntax will be valid
@@ -194,4 +196,5 @@ sub notSoSmartSplit { | |||
return $args; | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: extra line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bruh
@@ -2,6 +2,8 @@ package Logos::Generator::Base::Function; | |||
use strict; | |||
use Logos::Generator; | |||
use Logos::Util; | |||
use parent qw(Logos::Generator::Base::Common); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have been thrilled to receive and address any substantive feedback from all reviewers
I’m not coming back to this after months for a whitespace (respectfully)
What does this implement/fix? Explain your changes.
This PR adds the ability to specify inline blocks spanning multiple lines as arguments when calling
%orig(...)
, which was previously not supported.Example
%orig(...)
usages that are now supported:Does this close any currently open issues?
This PR closes an issue that's been open for 10y: [#6] "Logos errors out on calling %orig with an inline block with multiple lines."
Any relevant logs, error output, etc?
N/A
Any other comments?
%orig
,%log
, etc.) inside of a block passed to%orig(...)
is currently not supported.Example of unsupported usage:
Where has this been tested?
Operating System: macOS Sonoma, iOS 14, iOS 18, Ubuntu 22.0.4
Platform: arm64, x86_64
Target Platform: iOS, iOS Simulator
Toolchain Version: Xcode 15.4
SDK Version: iOS 14, iOS 15, iOS 16.5
Unit Testing
AFAIK, Logos (and Theos) lack official unit testing. This PR changes core behavior of the parser and has the potential to introduce significant breakage; this requires thorough testing to ensure reliability and backwards compatibility. To address this concern, I created a separate branch containing unit tests for my changes as well as some unrelated core behavior, as the project lacks these otherwise.
Coverage of these tests include, but is not limited to:
%orig
functionality to ensure backwards compatibility.%orig
usage.These tests validate the correctness of the current implementation and provide a safety net for future changes (helping prevent regressions). All of the tests I created can be found here.
Direct link to the tests for
%orig
behavior.I have Github Actions setup to run these tests (in my fork) -- they're currently passing: Latest Github Action run.