refactor: clarify uppercase exotic ident path #6779
Conversation
cometkim
changed the title
cometkim
force-pushed
the
perf-printidentlike
branch
2 times, most recently
from
5214d54 to
a3f202c
Compare
|
Does it look better to you? @cristianoc |
jscomp/syntax/src/res_printer.ml
Outdated
| | 'A' .. 'Z' when allowUident -> loop (i + 1) | ||
| | 'a' .. 'z' | '_' -> loop (i + 1) | ||
| | '-' when allowHyphen -> loop (i + 1) |
There was a problem hiding this comment.
why is this line removed?
There was a problem hiding this comment.
Oh thanks, I never intended to do this. (but tests are fine?)
There was a problem hiding this comment.
But, it seems we don't actually need this line because the scanner never makes an unwrapped ident with - as the first char I guess
| match classifyIdentContent ~allowUident:true txt with | ||
| | UppercaseExoticIdent -> |
There was a problem hiding this comment.
This cases unwraps then re-wraps the ident.
What's the net effect? Just to remove \ ?
There was a problem hiding this comment.
Yes it does.
Maybe we can replace it to
let len = String.length txt in
Doc.text ((String.sub [@doesNotRaise]) txt 1 (len - 1))There was a problem hiding this comment.
Either that, or add a comment that that's the effect, is fine.
Just, the current code can be a bit confusing without additional explanation.
There was a problem hiding this comment.
Btw, I don't know if it is possible, but one could try to classify the ident content already at scanning time, instead of print time.
And any id that is classified to be exotic could be represented internally as "id".
So the printer would need to do no special work.
And uppercase exotic ids would just be exotic ids that happen to start with capital letter -- nothing special.
There was a problem hiding this comment.
I had tried it within a previous PR to get as much work done from the scanner as possible, but it ultimately failed. I have no other ideas on how to do it while still using the existing parsetree as is. What do you mean "id"? Is there another place to mark it?
There was a problem hiding this comment.
Actually never mind, the scanner does not have the context that the printer has, such as when an uppercase identified is allowed.
Good to go for me.
There was a problem hiding this comment.
Left a comment about a possible different approach: https://github.com/rescript-lang/rescript-compiler/pull/6779/files#r1615712136
But I don't know if that would work, and is not for this PR anyway.
To me, this is good to go.
|
There are a few conflicts from the camelCase to snake_case PR. Once resolved, this can be merged. |
cometkim
force-pushed
the
perf-printidentlike
branch
from
a9a8372 to
636f978
Compare
cometkim
force-pushed
the
perf-printidentlike
branch
from
636f978 to
b0c6f1e
Compare
|
@cometkim Could you add a CHANGELOG entry? |
|
for this internal refactor...? |
|
We do have an "Internal" section in the CHANGELOG. |
|
Ok, never mind, I am currently cleaning up the CHANGELOG anyway and adding some missing stuff, will do that myself. |
This could improves perf in theory, but actually a bit slower... IDK whyIt is a slightly bit faster, but almost within the margin so... just for cleanup
(context: #6658 (comment))