Python: Improve computation of regex fragments inside string parts#14317
Python: Improve computation of regex fragments inside string parts#14317yoff merged 10 commits intogithub:mainfrom
Conversation
max-schaefer
left a comment
There was a problem hiding this comment.
Looks great, thanks!
| override Location getLocation() { result = StringPart_.super.getLocation() } | ||
|
|
||
| /** Holds if the content of string `StringPart` is surrounded by `prefix` and `quote`. */ | ||
| predicate context(string prefix, string quote) { |
There was a problem hiding this comment.
It seems like callers of this predicate are really only interested in the length of the prefix and the quote; perhaps it makes sense to just expose that?
There was a problem hiding this comment.
I did consider that; it might also be easier on the string pool. It just did not feel as generally useful, but perhaps that is a silly concern. (And given that the prefix and the quote are part of the string, it is actually the same information.)
| * `localOffset` will be the offset of this `RegExpTerm` inside `result`. | ||
| */ | ||
| StringPart getPart(int localOffset) { | ||
| exists(int index, int prefixLength | index = max(int i | this.getPartOffset(i) < start) | |
There was a problem hiding this comment.
Should this < be a <=?
There was a problem hiding this comment.
Hm, yes, I think so...good catch!
| ) | ||
| or | ||
| exists(StringPart part, int localOffset | part = this.getPart(localOffset) | | ||
| filepath = part.getLocation().getFile().getAbsolutePath() and |
There was a problem hiding this comment.
Have you considered using part.getLocation().hasLocationInfo (or perhaps even part.hasLocationInfo if it exists) to bind all of these variables in one go?
There was a problem hiding this comment.
Yeah, I went this way because it look clear, but actually going via hasLocationInfo is preferable also because it abstracts away that we choose the absolute path for the file.
Since we calculate the end column by offset, we must believ that the end line is the same as the start line.
tausbn
left a comment
There was a problem hiding this comment.
Sorry for the delay. Had to double-check that everything still made sense for StringParts arising from f-strings. (Luckily, it did.)
I think this looks good. 👍
No worries, that was exactly the kind of check, I was looking for 👍 |
No description provided.