-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
feat(compiler)!: Allow infix operators on new line #2136
base: main
Are you sure you want to change the base?
Conversation
alex-snezhko
commented
Aug 7, 2024
•
edited
Loading
edited
53dd993
to
eac5c75
Compare
eac5c75
to
86ed1d1
Compare
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'm not a fan of the changes to the parsetree. We should be able to have a fully semantic program without needing to read these extra properties, so that probably means we're not doing enough during parsing. We should be able to produce the error we want in the wrapped lexer.
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.
Mostly looks good to me, I had a few formatting nits.
riiiiiiightsInheriting | ||
& 1N << Module64.extendI32U(Module32.fromGrain(i) >> 1n) | ||
) | ||
> 0N |
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 feel like this formatting looks a little weird how would we feel about placing the operator on the top line in cases where a parens come first?
currentChar != '\u{00A0}' && // No Break Space | ||
currentChar != '\u{FEFF}' && // ZERO WIDTH NO-BREAK SPACE | ||
currentChar != '\u{0009}' | ||
&& // Tab |
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.
This comment formating doesn't seem right.
(val & Tags._GRAIN_GENERIC_TAG_MASK) == | ||
Tags._GRAIN_GENERIC_HEAP_TAG_TYPE | ||
(val & Tags._GRAIN_GENERIC_TAG_MASK) | ||
== Tags._GRAIN_GENERIC_HEAP_TAG_TYPE |
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.
Reading through all the formatted code, I find it looks really wierd with the ==
on a newline mostly because its not often that you chain equals together I wonder if we should preserve the old formatting style for ==
.
@@ -2184,7 +2184,8 @@ let trimString = (stringPtr: WasmI32, byteLength: WasmI32, fromEnd: Bool) => { | |||
// TODO(#661): Use unicode whitespace property and unicode line terminator | |||
if (!fromEnd) { | |||
if ( | |||
byte == 0xEFn && // Check for the first BOM byte | |||
byte == 0xEFn | |||
&& // Check for the first BOM byte |
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.
This comment being on the line below really feels like it changes the meaning of it. There has been a few cases like this.
@ospencer wouldn't that basically involve having an entire parser within wrapped_lexer? You'd need to know when you encounter a full expression within wrapped_lexer to know whether or not to give the warning |