[PATCH] D83592: [Parser] Add comment to skipped regions

[Vedant Kumar via Phabricator via llvm-commits]

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