vsk added a comment.
@zequanwu at a high level, I think this is the right approach and it looks nice
overall. I do have some feedback (inline). Thanks!
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:310
+ /// Return true if SR should be emitted as SkippedRange.
+ bool updateSkippedRange(SourceManager &SM, SpellingRegion &SR,
+ SourceRange Range) {
----------------
Could you restructure this to avoid mutating a SpellingRegion? This can aid
debugging (the unmodified struct remains available for inspection). One way to
do this might be to call the function "adjustSkippedRange" and have it return
an Optional<SpellingRegion>.
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:311
+ bool updateSkippedRange(SourceManager &SM, SpellingRegion &SR,
+ SourceRange Range) {
+ // If Range begin location is invalid, it's not a comment region.
----------------
What exactly is 'Range' in updateSkippedRange? It seems like it's the
{prev,next}TokLoc pair from SkippedRegion, but it's not obvious based on a
cursory read. It would help to rename 'Range' to something more specific
('InnermostNonCommentRange'? 'OverlappingExecutedTokenRange'?).
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.h:39
+ // A pair of SkippedRanges and PrevTokLoc with NextTokLoc
+ std::vector<std::pair<SourceRange, SourceRange>> SkippedRanges;
+ int LastCommentIndex = -1;
----------------
It'd be more self-descriptive to replace the pair with some `struct
SkippedRange { SourceRange SkippedRange; SourceLocation PrevTokLoc, NextTokLoc;
}`.
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.h:40
+ std::vector<std::pair<SourceRange, SourceRange>> SkippedRanges;
+ int LastCommentIndex = -1;
+
----------------
`Optional<unsigned> LastCommentIndex` would be slightly more idiomatic.
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.h:43
public:
- ArrayRef<SourceRange> getSkippedRanges() const { return SkippedRanges; }
+ SourceLocation PrevTokLoc;
+ SourceLocation BeforeCommentLoc;
----------------
Please leave short inline comments explaining the respective roles of
PrevTokLoc and BeforeCommentLoc.
================
Comment at: clang/test/lit.cfg.py:96
+config.substitutions.append(
+ ('%strip_comments', "sed 's/[ \t]*\/\/.*//' %s > %T/%basename_t")
+)
----------------
Bit of a nitpick, but I don't think it'll necessarily end up being more
convenient to have "%T/%basename_t" hardcoded in this substitution (strawman,
but: you can imagine a test that needs to strip comments from two different
files). It'd be nice to be more explicit and write the run lines like:
RUN: %strip_comments %s > %t.stripped.c
RUN: clang ... %t.stripped.c
================
Comment at: compiler-rt/test/profile/Linux/coverage_comments.cpp:1
+// RUN: %clangxx_profgen -std=c++11 -fuse-ld=gold -fcoverage-mapping
-Wno-comment -o %t %s
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
----------------
I don't think there's anything Linux or gold-specific about this test. Could
you move the test up one directory, so we can run it on Darwin/Windows/etc.?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83592/new/
https://reviews.llvm.org/D83592
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
