Improving multiline context snippet #2288
Conversation
src/Sarif/FileRegionsCache.cs
Outdated
| if (inputRegion.Snippet?.Text.Length >= reasonableSnippetLength) | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
shouldn't we actually be checking the region size, not the snippet size? basically, if a region exceeds a certain size, no context region is required. so for this case, the region should have the snippet. your code will permit the following case:
region is 10k in size but has no snippet
we then create a context region + snippet which is smaller than the region! this breaks the SARIF spec. :) #Resolved
There was a problem hiding this comment.
src/Sarif/FileRegionsCache.cs
Outdated
| const int smallSnippetLength = 128; | ||
|
|
||
| // If this is big enough, we don't need to create a new snippet. | ||
| if (inputRegion.CharLength >= reasonableSnippetLength) |
There was a problem hiding this comment.
inputRegion.CharLength [](start = 16, length = 22)
Can we please put a check in place that ensures the region has populated char length and offset properly? Go check the spec on this type, there's a sentinel value/property default that you can use to verify.
There was a problem hiding this comment.
Ok, so, we will just check this value if charOffset != -1.
In reply to: 578571743 [](ancestors = 578571743)
There was a problem hiding this comment.
Wait, this isn't right, is it? If CharOffset == -1, this method can't function, can it?
In reply to: 578583524 [](ancestors = 578583524,578571743)
There was a problem hiding this comment.
if == -1, then, ur region is using startline / startcolumn, with that, the populateTextRegion handle both cases.
In reply to: 578586111 [](ancestors = 578586111,578583524,578571743)
src/Sarif/FileRegionsCache.cs
Outdated
| region.CharLength = inputRegion.CharLength + inputRegion.CharOffset + smallSnippetLength < newLineIndex.Text.Length | ||
| ? inputRegion.CharLength + inputRegion.CharOffset + smallSnippetLength | ||
| : newLineIndex.Text.Length - region.CharOffset; | ||
|
|
||
| return this.PopulateTextRegionProperties(region, uri, populateSnippet: true); |
There was a problem hiding this comment.
return [](start = 12, length = 6)
the end of this method could be a good place to introduce a debug assert that verifies we have limited the size of the region that we return. #Resolved
There was a problem hiding this comment.
Added the debug.Assert to verify if the new length >= the original
In reply to: 578572270 [](ancestors = 578572270)
Change