-
Notifications
You must be signed in to change notification settings - Fork 25
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
Improve parsing of SPL2 modules for statement names to handle strings, fields, functions, comments. #131
Conversation
out/notebooks/utils/parsing.ts
Outdated
newModule += next; | ||
} | ||
} | ||
spl2Module = spl2Module.substring(indx, spl2Module.length); |
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 might be a tall ask and is out of scope for this, but are the repeated substring calls suboptimal here? Would it be best to traverse the string via an index like so:
newModule = "";
for (index = 0; index < spl2Module.length; index++) {
next = spl2Module[indx];
...
// Case logic and all here
...
if (!inBlockComment && !inField && !inString && !inLineComment && braceLevel === 0) {
newModule += next;
prev = next;
}
Doing so might fare better for larger modules since we don't need to repeatedly substringify the module.
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.
Yes, good suggestion substring
made more sense before I rewrote the parsing code but serves no purpose anymore. I made your suggested changes.
let braceLevel = 0; // { .. } | ||
|
||
let newModule = ''; | ||
let prev = ''; |
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.
Where are we updating prev
? I don't see any assignments to prev
, specifically nothing like prev = next
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.
great catch, it looks like I neglected to add a test that includes string and field escape chars too so let me do that
Approved on my end! |
This PR adds more robust parsing for search statements to exclude cases like local variable assignments within a function:
or statements that have been commented out: