coverage: Don't underflow column number#97368
Conversation
I noticed this when running coverage on a debug build of rustc. There may be other places that do this but I'm just fixing the one I hit.
| let CharPos(char_pos) = start_col; | ||
| if span.hi() == body_span.hi() { | ||
| start_col = CharPos(char_pos - 1); | ||
| start_col = CharPos(char_pos.saturating_sub(1)); |
There was a problem hiding this comment.
I'm wondering if we should fix the rest of this function to use saturating math since it's not much code? Specifically, can we also fix the addition on lines 520, 531 and 533?
There was a problem hiding this comment.
@tmandry - Do you mind doing that, if you agree it's worth it?
Sorry for the bug, but I thought lookup_file_pos() should never return less than 1. Maybe I misunderstood or the implementation changed, but that's not something the compiler would have enforced of course, so this is a good fix. Thanks.
| let CharPos(char_pos) = start_col; | ||
| if span.hi() == body_span.hi() { | ||
| start_col = CharPos(char_pos - 1); | ||
| start_col = CharPos(char_pos.saturating_sub(1)); |
There was a problem hiding this comment.
@tmandry - Do you mind doing that, if you agree it's worth it?
Sorry for the bug, but I thought lookup_file_pos() should never return less than 1. Maybe I misunderstood or the implementation changed, but that's not something the compiler would have enforced of course, so this is a good fix. Thanks.
|
Ping from triage: |
|
This seems like a strict improvement to me - @wesleywiser @richkadel feel free to open a follow-up issue if you feel strongly about using saturating arithmetic for the addition too. @bors r+ rollup |
|
mmm coverage tests are somewhat likely to fail I'm remembering @bors rollup=maybe |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (8763965): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
I noticed this when running coverage on a debug build of rustc. There
may be other places that do this but I'm just fixing the one I hit.
r? @wesleywiser @richkadel