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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

The testing looks really good. Just a few more requests for documentation (inline).



================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:309
 
+  /// Return a new SpellingRegion for the SkippedRange if it's valid.
+  Optional<SpellingRegion> adjustSkippedRange(SourceManager &SM,
----------------
This could use some description of what adjustment is done, e.g. "This shrinks the skipped range if it spans a line that contains a non-comment token. If shrinking the skipped range would make it empty, this returns None."


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.h:37
+  SourceLocation PrevTokLoc;
+  SourceLocation NextTokLoc;
+
----------------
It'd be helpful to leave inline documentation for these fields as well. It's clear from context that 'Range' is the skipped source range, but it's not as clear what {Prev,Next}TokLoc are.


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.h:48
+class CoverageSourceInfo : public PPCallbacks, public CommentHandler {
+  // A pair of SkippedRanges and PrevTokLoc with NextTokLoc
+  std::vector<SkippedRange> SkippedRanges;
----------------
Please end sentences with a '.'


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.h:53
 public:
-  ArrayRef<SourceRange> getSkippedRanges() const { return SkippedRanges; }
+  // The location of previously token. It's updated when Preprocessor::Lex
+  // lexed a new token.
----------------
How does "Location of the token parsed before HandleComment is called. This is updated every time Preprocessor::Lex lexes a new token." sound?


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

https://reviews.llvm.org/D83592