C++: Small locations fix for regex PR - #33
Conversation
| or | ||
| // No opening '"'. | ||
| not re instanceof RawStringLiteral and | ||
| not exists(int i | vt.charAt(i) = "\"") and | ||
| result = 0 |
There was a problem hiding this comment.
Don't we have the same problem for raw string literals, but then for (?
There was a problem hiding this comment.
If it's possible to stringify into raw string literals, yes.
There was a problem hiding this comment.
... in fact right now no, due to the way you define RawStringLiteral it must contain a (. But it's fragile to depend on that.
There was a problem hiding this comment.
It would be less fragile if we defined getContentOffset() in StringLiteral and RawStringLiteral. That way the definition that depends on ( being present is close to the constructor that ensures it, so it seems less fragile as a future developer working on RawStringLiteral ought to be aware of both.
How do you feel about the direction of providing the functionality of regexpContentOffset in StringLiteral?
2 participants
PR onto github#22300 .
I was a bit skeptical that the second case in
regexpContentOffset()can safely assume aStringLiteral.getValueTextnecessarily always contains". A quick real world investigation confirmed this isn't always true - in particular, string literals produced by macro stringification don't necessarily contain a"in their value text.This PR adds a few test cases and addresses the gap in locations. Note that the new locations are not character accurate - but they do at least have a location on the correct line of code, sufficient for a user to see what is being referred to (doing better might be tricky and has debatable priority).
I think we should also consider moving most of the logic from
regexpContentOffset()into a methodStringLiteral.getContentOffset(), overridden inRawStringLiteralfor the additional logic of that situation. I haven't done this here.