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

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

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

zequanwu updated this revision to Diff 279395. zequanwu added a comment. Missed something. Updated CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 Files: clang/include/clang/Lex/Preprocessor.h clang/lib/CodeGen/CodeGenAction.cpp

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

zequanwu updated this revision to Diff 279394. zequanwu marked 7 inline comments as done. zequanwu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 Files: clang/include/clang/Lex/Preprocessor.h

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

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

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

zequanwu updated this revision to Diff 279331. zequanwu added a comment. `arc diff --update` automatically formatted test case. Reverted. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 Files: clang/include/clang/Lex/Preprocessor.h

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

zequanwu updated this revision to Diff 279329. zequanwu added a comment. Herald added a project: Sanitizers. Herald added a subscriber: Sanitizers. Rebase and address comments. - Keep track of token locations before and after comment regions by `onToken` in `Preprocessor` so when emitting

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

zequanwu added a comment. In D83592#2157130 , @vsk wrote: > > ! In D83592#2156758 , @zequanwu > > wrote: > > One way I could think is probably when we visit decl in > > `CounterCoverageMappingBuilder`, check for

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

vsk added a comment. > ! In D83592#2156758 , @zequanwu > wrote: > One way I could think is probably when we visit decl in > `CounterCoverageMappingBuilder`, check for if there is `SkippedRegion` in the > same line and then mark the decl range as

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

zequanwu added a comment. In D83592#2151773 , @zequanwu wrote: > In D83592#2151217 , @vsk wrote: > > > Could you add an end-to-end llvm-cov test (see e.g. > > compiler-rt/test/profile/Linux/coverage_ctors.cpp)?

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

zequanwu added a comment. In D83592#2151217 , @vsk wrote: > Could you add an end-to-end llvm-cov test (see e.g. > compiler-rt/test/profile/Linux/coverage_ctors.cpp)? Here are some important > cases I think we should check: > > - `/* comment at the start

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

zequanwu updated this revision to Diff 277988. zequanwu added a comment. `%strip_comments` -> `sed 's/[ \t]*\/\/.*//' %s > %T/%basename_t`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 Files:

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

zequanwu updated this revision to Diff 277986. zequanwu added a comment. Add lit substitution `%strip_comments -> sed 's/\/\/.*//' %s > %T/%basename_t`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592

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

vsk added a comment. Could you add an end-to-end llvm-cov test (see e.g. compiler-rt/test/profile/Linux/coverage_ctors.cpp)? Here are some important cases I think we should check: - `/* comment at the start of a line */ expr;` - `expr; /* comment at the end of a line */` - `expr; // comment at

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

zequanwu updated this revision to Diff 277909. zequanwu added a comment. Fix failed test cases with sed to remove comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 Files:

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

hans added a comment. > The comments in those coverage tests are used for FileCheck, like `//CHECK:`. > So, we can't remove those ones. Oh, I didn't think about that :-) It's a bit unusual and annoying that the test expectations end up affecting the test output. For the comments that are not

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

zequanwu added a comment. For those test cases, I could either moving the comments inside a function to outside or change `CHECK-NEXT` to `CHECK` In D83592#2148833 , @vsk wrote: > Before updating any tests, maybe it's worth doing a quick experiment with

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

vsk added a comment. Before updating any tests, maybe it's worth doing a quick experiment with comments placed in different contexts, to see whether adding these skipped regions is really sufficient. For example, given: 1| for (auto x : collection) { 2|// Explain the loop. 3| }

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

zequanwu updated this revision to Diff 277611. zequanwu added a comment. Accidentally uploaded binary.. removed now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 Files:

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

zequanwu updated this revision to Diff 277606. zequanwu added a comment. Remove old code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592 Files: a.out a.profdata clang/lib/CodeGen/CodeGenAction.cpp

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

zequanwu marked an inline comment as done. zequanwu added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:152 +static MutableArrayRef +mergeSkippedRegions(MutableArrayRef MappingRegions) { + SmallVector MergedRegions; vsk

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

zequanwu updated this revision to Diff 277605. zequanwu added a comment. Vedant, thanks for your suggestions. - Remove merging function. - Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/ https://reviews.llvm.org/D83592

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

vsk added a comment. In D83592#2148638 , @zequanwu wrote: > In D83592#2148376 , @vsk wrote: > > > taking advantage of the existing CommentHandler interface? To simplify > > things, we can add a static method like

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

zequanwu marked an inline comment as done. zequanwu added a comment. In D83592#2148376 , @vsk wrote: > taking advantage of the existing CommentHandler interface? To simplify > things, we can add a static method like >

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

vsk added a comment. @zequanwu thanks for working on this. Instead of threading CommentSkipped through PPCallbacks, wdyt of taking advantage of the existing CommentHandler interface? To simplify things, we can add a static method like

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

zequanwu updated this revision to Diff 277538. zequanwu added a comment. Herald added subscribers: llvm-commits, hiraditya. Herald added a project: LLVM. Merge overlapped `SkippedRegion`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83592/new/

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

zequanwu added a comment. In D83592#2147881 , @hans wrote: > > Not sure if this is good idea to untrack comments, it breaks many tests > > under CoverageMapping. > > I didn't look, but I'm surprised there are many coverage tests that have > comments in

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

zequanwu added inline comments. Herald added a subscriber: wuzish. Comment at: clang/lib/Parse/Parser.cpp:37 bool HandleComment(Preprocessor , SourceRange Comment) override { +PP.getPPCallbacks()->SourceRangeSkipped(Comment, Comment.getEnd());

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

zequanwu updated this revision to Diff 277518. zequanwu marked an inline comment as done. zequanwu added a comment. Herald added subscribers: kbarton, nemanjai. Classfiying comments as `SkippedRegion`,

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

hans added a comment. > Not sure if this is good idea to untrack comments, it breaks many tests under > CoverageMapping. I didn't look, but I'm surprised there are many coverage tests that have comments in them. If the comments are not important for those tests, maybe they could be removed?

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

zequanwu created this revision. zequanwu added a reviewer: hans. Herald added a project: clang. Herald added a subscriber: cfe-commits. Not sure if this is good idea to untrack comments, it breaks many tests under CoverageMapping. Repository: rG LLVM Github Monorepo