-
Notifications
You must be signed in to change notification settings - Fork 624
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
Fix promises at EOF #4097
base: master
Are you sure you want to change the base?
Fix promises at EOF #4097
Conversation
The previous computation could result in invalid values, which could lead to invalid promise ranges and subsequent crashes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4097 +/- ##
=======================================
Coverage 85.72% 85.72%
=======================================
Files 239 239
Lines 56961 56963 +2
=======================================
+ Hits 48827 48829 +2
Misses 8134 8134 ☔ View full report in Codecov by Sentry. |
Thank you.
I agree with you. Some issues I have opened these days are related to this area. I will review this. |
body input.php /^<div id="body">$/;" I line:26 language:HTML roles:def | ||
content input.php /^ <main id="content">$/;" I line:27 language:HTML roles:def | ||
post input.php /^ <article class="post" id="post-<?p/;" c line:31 language:HTML roles:attribute | ||
post-<?p input.php /^ <article class="post" id="post-<?p/;" I line:31 language:HTML roles:def |
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 have a follow-up to fix the HTML promise range from the PHP lexer to fix this spurious <?p
, but it currently depends on these changes so I'll post it after this is sorted out (no pressure, just saying that indeed, this tag is not exactly as it should be).
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.
Could you paste this comment into the commit log for this commit?
So people can understand this commit from its log only with 'git log'.
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.
If you want, but I'm not sure its so important to specify in the commit, it's just a note for review that it's not to be worried about, but I don't think it undermines the patch much.
Another option could be removing this line from the test case and add a new one for the other patch, I merely kept it like this because it's a "real" file and made me notice the issue.
Anyway, I can do either just fine, as you prefer :)
getInputLineOffset()
was giving very wrong results at EOF, leading to overlong promises, leading to crashes in building the narrowed input stream (see e.g. newly introduced test case, and geany/geany#3939).I'm unsure if this is a good way to fix the issue, but I couldn't find a better one; but admittedly I didn't poke around this code for a long time, and it got a fair bit more complicated with its newer features, so I might have missed something.