[PATCH] D84988: [Coverage] Add empty line regions to SkippedRegions

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:326
+    if (PrevTokLoc.isValid()) {
+      unsigned PrevTokLine = SM.getSpellingLineNumber(PrevTokLoc);
+      if (SR.LineStart == PrevTokLine) {
----------------
vsk wrote:
> It looks like this assumes there is some guarantee that the skipped range (as given by SR) is in the same file as {Prev,Next}TokLoc. If that's correct, can we go ahead and `assert` that?
Oh, it's a bug in https://reviews.llvm.org/D83592. There is no guarantee that they are in the same file. 


================
Comment at: clang/lib/Lex/Lexer.cpp:3290
       IsAtPhysicalStartOfLine = true;
+      NewLinePtr = CurPtr - 1;
 
----------------
vsk wrote:
> Is NewLinePtr supposed to point to the start of the most recent newline sequence? For "\r\n", is it supposed to be "\r<NewLinePtr>\n" or "<NewLinePtr>\r\n"?
I didn't consider this. Updated.
NewLinePtr is supposed to point to the '\n' character. For "\r\n", it will point to '\n'.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:483
       bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion;
 
       if (CR.index() + 1 == Regions.size() ||
----------------
vsk wrote:
> Why is this deletion necessary? Do we need to introduce 0-length regions which alter the count? An example would help.
Because a single empty line will be a 0 length region. I don't know why is this condition necessary before. Does zero-length region exists before this change?

example:
```
int main() {

  return 0;
}
```
Before, llvm-cov gives the following.
```
Counter in file 0 1:12 -> 4:2, #0
Counter in file 0 2:1 -> 2:1, 0
Emitting segments for file: /tmp/a.c
Combined regions:
  1:12 -> 4:2 (count=1)
  2:1 -> 2:1 (count=0)
Segment at 1:12 (count = 1), RegionEntry
Segment at 2:1 (count = 0), RegionEntry, Skipped
Segment at 4:2 (count = 0), Skipped
    1|      1|int main() {
    2|       |
    3|       |    return 0;
    4|       |}
```
After:
```
Counter in file 0 1:12 -> 4:2, #0
Counter in file 0 2:1 -> 2:1, 0
Emitting segments for file: /tmp/a.c
Combined regions:
  1:12 -> 4:2 (count=1)
  2:1 -> 2:1 (count=0)
Segment at 1:12 (count = 1), RegionEntry
Segment at 2:1 (count = 0), RegionEntry, Skipped
Segment at 2:1 (count = 1)
Segment at 4:2 (count = 0), Skipped
    1|      1|int main() {
    2|       |
    3|      1|    return 0;
    4|      1|}
```


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:580
       const auto &R = Segments[I];
-      if (!(L.Line < R.Line) && !(L.Line == R.Line && L.Col < R.Col)) {
+      if (!(L.Line <= R.Line) && !(L.Line == R.Line && L.Col <= R.Col)) {
         LLVM_DEBUG(dbgs() << " ! Segment " << L.Line << ":" << L.Col
----------------
vsk wrote:
> I don't think this relaxation is correct, since it permits duplicate segments. This is confusing for reporting tools because it's not possible to work out which segment applies at a given source location.
I don't remember why I made this change. Reverting it seems nothing changed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84988/new/

https://reviews.llvm.org/D84988