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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits