-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Avoid trying to interpret escape sequences inside comments. #95
Conversation
# In case of error, just let's pick the code | ||
# from the `input_line` attribute of | ||
# prescanner: | ||
self.code = self.prescanner.input_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.
Actually, we can replace the whole try/except block by lines 476 and 477.
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 that works, it would be simpler.
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 tested against the comment that produces the syntax error in GS1.m
and works. Then I tested on each GS?.m
and we still have one test wrong on GS2.m
and GS3.m
, but it seems it is not related to this issue.
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.
Looking again, at found a problem with the "simpler" approach: it could happen that after the comment finishes, some code that needs to take into account the escape characters comes. For example,
(*
Hi, this is a test \|<-this would produce an error if we pass by `replace_escape_sequences`
*)
a="\|AAAAAA"
(*The previous line would require the use of replace_escape_sequences*)
So I was wrong with the extra simplification.
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.
Still this fix does not cover a corner case of the form
(* corner case *) A="\|AAAAAA"
So maybe we need to track this, and reload the last line. I will change the comment to mention this.
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.
All of these approaches feel hacky and not the way scanners usually work. Handling escape sequences is not something that is typically done separately from initial scanning. But right now this is not something I want to spend time on.
So right now I would prefer defer addressing until such time when we can do this properly. The code base has too many "quick hacks" and could use better-thought-out and more standard solutions.
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.
So maybe we need to track this, and reload the last line. I will change the comment to mention this.
Yes, please open an issue for this. Long comments on specific bugs are better put in the issue tracker. There, one can be extremely verbose, more so than in comments.
LGTM - merge when you are satisfied. Let's leave the more-complete rework for later though. |
This should fix #94