subject:"\[PATCH\] D83592\: \[Parser\] Add comment to skipped regions"

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

2020-07-21 Thread Vedant Kumar via Phabricator via cfe-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 adjustSkippedRange(SourceManager ,

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 SkippedRanges;

Please end sentences with a '.'



Comment at: clang/lib/CodeGen/CoverageMappingGen.h:53
 public:
-  ArrayRef 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2020-07-20 Thread Zequan Wu via Phabricator via cfe-commits

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
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Preprocessor.cpp
  clang/test/CoverageMapping/break.c
  clang/test/CoverageMapping/builtinmacro.c
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/comment-in-macro.c
  clang/test/CoverageMapping/continue.c
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/if.cpp
  clang/test/CoverageMapping/includehell.cpp
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/logical.cpp
  clang/test/CoverageMapping/loops.cpp
  clang/test/CoverageMapping/macro-expressions.cpp
  clang/test/CoverageMapping/macroparams2.c
  clang/test/CoverageMapping/macros.c
  clang/test/CoverageMapping/macroscopes.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/objc.m
  clang/test/CoverageMapping/pr32679.cpp
  clang/test/CoverageMapping/preprocessor.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/test.c
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  clang/test/CoverageMapping/while.c
  clang/test/lit.cfg.py
  compiler-rt/test/profile/Inputs/instrprof-comdat.h
  compiler-rt/test/profile/coverage_comments.cpp

Index: compiler-rt/test/profile/coverage_comments.cpp
===
--- /dev/null
+++ compiler-rt/test/profile/coverage_comments.cpp
@@ -0,0 +1,71 @@
+// RUN: %clangxx_profgen -fcoverage-mapping -Wno-comment -o %t %s
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile %t.profdata -path-equivalence=/tmp,%S 2>&1 | FileCheck %s
+
+int main() {   // CHECK:  [[@LINE]]| 1|int main() {
+/* comment */ int x = 0;   // CHECK:  [[@LINE]]| 1|  /* comment */ int x = 0;
+int y = 0; /* comment */   // CHECK:  [[@LINE]]| 1|  int y = 0; /* comment */
+int z = 0; // comment  // CHECK:  [[@LINE]]| 1|  int z = 0; // comment
+// comment // CHECK:  [[@LINE]]|  |  // comment
+   // CHECK:  [[@LINE]]|  |
+x = 0; /*  // CHECK:  [[@LINE]]| 1|  x = 0; /*
+comment// CHECK:  [[@LINE]]|  |comment
+*/ // CHECK:  [[@LINE]]|  |*/
+   // CHECK:  [[@LINE]]|  |
+/* // CHECK:  [[@LINE]]|  |/*
+comment// CHECK:  [[@LINE]]|  |comment
+*/ x = 0;  // CHECK:  [[@LINE]]| 1|*/ x = 0;
+   // CHECK:  [[@LINE]]|  |
+/* comment */  // CHECK:  [[@LINE]]|  |/* comment */
+// comment // CHECK:  [[@LINE]]|  |// comment
+/* comment */  // CHECK:  [[@LINE]]|  |/* comment */
+z =// CHECK:  [[@LINE]]| 1|z =
+x // comment   // CHECK:  [[@LINE]]| 1|x // comment
+// comment // CHECK:  [[@LINE]]|  |// comment
++ /*   // CHECK:  [[@LINE]]| 1|+ /*
+comment// CHECK:  [[@LINE]]|  |comment
+*/ // CHECK:  [[@LINE]]|  |*/
+/* // CHECK:  [[@LINE]]|  |/*
+comment// CHECK:  [[@LINE]]|  |comment
+*/y;   // CHECK:  [[@LINE]]| 1|*/y;
+   // CHECK:  [[@LINE]]|  |
+// Comments inside directives. // CHECK:  [[@LINE]]|  |// Comments inside directives.
+#if 0 //comment// CHECK:  [[@LINE]]|  |#if 0 //comment
+/* comment */ x = 0;   // CHECK:  [[@LINE]]|  |/* comment */ x = 0;
+y = 0; /* comment */   // CHECK:  [[@LINE]]|  |y = 0; /* comment */
+z = 0; // comment  // CHECK:  [[@LINE]]|  |z = 0; // comment
+// comment // CHECK:  [[@LINE]]|  |// comment
+   // CHECK:  [[@LINE]]|  |
+x = 0; /*  // CHECK:  [[@LINE]]|  |x = 0; /*
+comment// CHECK:  [[@LINE]]|  |comment
+*/ // CHECK:  [[@LINE]]|  |

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

2020-07-20 Thread Zequan Wu via Phabricator via cfe-commits

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
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Preprocessor.cpp
  clang/test/CoverageMapping/break.c
  clang/test/CoverageMapping/builtinmacro.c
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/comment-in-macro.c
  clang/test/CoverageMapping/continue.c
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/if.cpp
  clang/test/CoverageMapping/includehell.cpp
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/logical.cpp
  clang/test/CoverageMapping/loops.cpp
  clang/test/CoverageMapping/macro-expressions.cpp
  clang/test/CoverageMapping/macroparams2.c
  clang/test/CoverageMapping/macros.c
  clang/test/CoverageMapping/macroscopes.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/objc.m
  clang/test/CoverageMapping/pr32679.cpp
  clang/test/CoverageMapping/preprocessor.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/test.c
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  clang/test/CoverageMapping/while.c
  clang/test/lit.cfg.py
  compiler-rt/test/profile/Inputs/instrprof-comdat.h
  compiler-rt/test/profile/coverage_comments.cpp

Index: compiler-rt/test/profile/coverage_comments.cpp
===
--- /dev/null
+++ compiler-rt/test/profile/coverage_comments.cpp
@@ -0,0 +1,71 @@
+// RUN: %clangxx_profgen -fcoverage-mapping -Wno-comment -o %t %s
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile %t.profdata -path-equivalence=/tmp,%S 2>&1 | FileCheck %s
+
+int main() {   // CHECK:  [[@LINE]]| 1|int main() {
+/* comment */ int x = 0;   // CHECK:  [[@LINE]]| 1|  /* comment */ int x = 0;
+int y = 0; /* comment */   // CHECK:  [[@LINE]]| 1|  int y = 0; /* comment */
+int z = 0; // comment  // CHECK:  [[@LINE]]| 1|  int z = 0; // comment
+// comment // CHECK:  [[@LINE]]|  |  // comment
+   // CHECK:  [[@LINE]]|  |
+x = 0; /*  // CHECK:  [[@LINE]]| 1|  x = 0; /*
+comment// CHECK:  [[@LINE]]|  |comment
+*/ // CHECK:  [[@LINE]]|  |*/
+   // CHECK:  [[@LINE]]|  |
+/* // CHECK:  [[@LINE]]|  |/*
+comment// CHECK:  [[@LINE]]|  |comment
+*/ x = 0;  // CHECK:  [[@LINE]]| 1|*/ x = 0;
+   // CHECK:  [[@LINE]]|  |
+/* comment */  // CHECK:  [[@LINE]]|  |/* comment */
+// comment // CHECK:  [[@LINE]]|  |// comment
+/* comment */  // CHECK:  [[@LINE]]|  |/* comment */
+z =// CHECK:  [[@LINE]]| 1|z =
+x // comment   // CHECK:  [[@LINE]]| 1|x // comment
+// comment // CHECK:  [[@LINE]]|  |// comment
++ /*   // CHECK:  [[@LINE]]| 1|+ /*
+comment// CHECK:  [[@LINE]]|  |comment
+*/ // CHECK:  [[@LINE]]|  |*/
+/* // CHECK:  [[@LINE]]|  |/*
+comment// CHECK:  [[@LINE]]|  |comment
+*/y;   // CHECK:  [[@LINE]]| 1|*/y;
+   // CHECK:  [[@LINE]]|  |
+// Comments inside directives. // CHECK:  [[@LINE]]|  |// Comments inside directives.
+#if 0 //comment// CHECK:  [[@LINE]]|  |#if 0 //comment
+/* comment */ x = 0;   // CHECK:  [[@LINE]]|  |/* comment */ x = 0;
+y = 0; /* comment */   // CHECK:  [[@LINE]]|  |y = 0; /* comment */
+z = 0; // comment  // CHECK:  [[@LINE]]|  |z = 0; // comment
+// comment // CHECK:  [[@LINE]]|  |// comment
+   // CHECK:  [[@LINE]]|  |
+x = 0; /*  // CHECK:  [[@LINE]]|  |x = 0; /*
+comment// CHECK:  [[@LINE]]|  |comment
+*/

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

2020-07-20 Thread Vedant Kumar via Phabricator via cfe-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 , SpellingRegion ,
+  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.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:311
+  bool updateSkippedRange(SourceManager , SpellingRegion ,
+  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> 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> SkippedRanges;
+  int LastCommentIndex = -1;
+

`Optional LastCommentIndex` would be slightly more idiomatic.



Comment at: clang/lib/CodeGen/CoverageMappingGen.h:43
 public:
-  ArrayRef 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

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

2020-07-20 Thread Zequan Wu via Phabricator via cfe-commits

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
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Preprocessor.cpp
  clang/test/CoverageMapping/break.c
  clang/test/CoverageMapping/builtinmacro.c
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/comment-in-macro.c
  clang/test/CoverageMapping/continue.c
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/if.cpp
  clang/test/CoverageMapping/includehell.cpp
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/logical.cpp
  clang/test/CoverageMapping/loops.cpp
  clang/test/CoverageMapping/macro-expressions.cpp
  clang/test/CoverageMapping/macroparams2.c
  clang/test/CoverageMapping/macros.c
  clang/test/CoverageMapping/macroscopes.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/objc.m
  clang/test/CoverageMapping/pr32679.cpp
  clang/test/CoverageMapping/preprocessor.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/test.c
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  clang/test/CoverageMapping/while.c
  clang/test/lit.cfg.py
  compiler-rt/test/profile/Inputs/instrprof-comdat.h
  compiler-rt/test/profile/Linux/coverage_comments.cpp

Index: compiler-rt/test/profile/Linux/coverage_comments.cpp
===
--- /dev/null
+++ compiler-rt/test/profile/Linux/coverage_comments.cpp
@@ -0,0 +1,71 @@
+// 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
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile %t.profdata -path-equivalence=/tmp,%S 2>&1 | FileCheck %s
+
+int main() {   // CHECK:  [[@LINE]]| 1|int main() {
+/* comment */ int x = 0;   // CHECK:  [[@LINE]]| 1|  /* comment */ int x = 0;
+int y = 0; /* comment */   // CHECK:  [[@LINE]]| 1|  int y = 0; /* comment */
+int z = 0; // comment  // CHECK:  [[@LINE]]| 1|  int z = 0; // comment
+// comment // CHECK:  [[@LINE]]|  |  // comment
+   // CHECK:  [[@LINE]]|  |
+x = 0; /*  // CHECK:  [[@LINE]]| 1|  x = 0; /*
+comment// CHECK:  [[@LINE]]|  |comment
+*/ // CHECK:  [[@LINE]]|  |*/
+   // CHECK:  [[@LINE]]|  |
+/* // CHECK:  [[@LINE]]|  |/*
+comment// CHECK:  [[@LINE]]|  |comment
+*/ x = 0;  // CHECK:  [[@LINE]]| 1|*/ x = 0;
+   // CHECK:  [[@LINE]]|  |
+/* comment */  // CHECK:  [[@LINE]]|  |/* comment */
+// comment // CHECK:  [[@LINE]]|  |// comment
+/* comment */  // CHECK:  [[@LINE]]|  |/* comment */
+z =// CHECK:  [[@LINE]]| 1|z =
+x // comment   // CHECK:  [[@LINE]]| 1|x // comment
+// comment // CHECK:  [[@LINE]]|  |// comment
++ /*   // CHECK:  [[@LINE]]| 1|+ /*
+comment// CHECK:  [[@LINE]]|  |comment
+*/ // CHECK:  [[@LINE]]|  |*/
+/* // CHECK:  [[@LINE]]|  |/*
+comment// CHECK:  [[@LINE]]|  |comment
+*/y;   // CHECK:  [[@LINE]]| 1|*/y;
+   // CHECK:  [[@LINE]]|  |
+// Comments inside directives. // CHECK:  [[@LINE]]|  |// Comments inside directives.
+#if 0 //comment// CHECK:  [[@LINE]]|  |#if 0 //comment
+/* comment */ x = 0;   // CHECK:  [[@LINE]]|  |/* comment */ x = 0;
+y = 0; /* comment */   // CHECK:  [[@LINE]]|  |y = 0; /* comment */
+z = 0; // comment  // CHECK:  [[@LINE]]|  |z = 0; // comment
+// comment // CHECK:  [[@LINE]]|  |// comment
+   // CHECK:  [[@LINE]]|  |
+x = 0; /*  // CHECK:  [[@LINE]]|  |x = 0; /*
+comment// CHECK:

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

2020-07-20 Thread Zequan Wu via Phabricator via cfe-commits

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 `SkippedRegion` we can determine if the 
region should be emitted or not.
- Add an end-to-end llvm-cov test case.


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
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Preprocessor.cpp
  clang/test/CoverageMapping/break.c
  clang/test/CoverageMapping/builtinmacro.c
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/comment-in-macro.c
  clang/test/CoverageMapping/continue.c
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/if.cpp
  clang/test/CoverageMapping/includehell.cpp
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/logical.cpp
  clang/test/CoverageMapping/loops.cpp
  clang/test/CoverageMapping/macro-expressions.cpp
  clang/test/CoverageMapping/macroparams2.c
  clang/test/CoverageMapping/macros.c
  clang/test/CoverageMapping/macroscopes.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/objc.m
  clang/test/CoverageMapping/pr32679.cpp
  clang/test/CoverageMapping/preprocessor.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/test.c
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  clang/test/CoverageMapping/while.c
  clang/test/lit.cfg.py
  compiler-rt/test/profile/Inputs/instrprof-comdat.h
  compiler-rt/test/profile/Linux/coverage_comments.cpp

Index: compiler-rt/test/profile/Linux/coverage_comments.cpp
===
--- /dev/null
+++ compiler-rt/test/profile/Linux/coverage_comments.cpp
@@ -0,0 +1,77 @@
+// 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
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile %t.profdata -path-equivalence=/tmp,%S 2>&1 | FileCheck %s
+
+int main() {   // CHECK:  [[@LINE]]| 1|int main() {
+  /* comment */ int x = 0; // CHECK:  [[@LINE]]| 1|  /* comment */ int x = 0;
+  int y = 0; /* comment */ // CHECK:  [[@LINE]]| 1|  int y = 0; /* comment */
+  int z = 0;   // comment  // CHECK:  [[@LINE]]| 1|  int z = 0; // comment
+  // comment // CHECK:  [[@LINE]]|  |  // comment
+  // CHECK:  [[@LINE]]|  |
+  x = 0;/*  // CHECK:  [[@LINE]]| 1|  x = 0; /*
+comment// CHECK:  [[@LINE]]|  |comment
+*/
+// CHECK:  [[@LINE]]|  |*/
+// CHECK:  [[@LINE]]|  |
+/* // CHECK:  [[@LINE]]|  |/*
+comment// CHECK:  [[@LINE]]|  |comment
+*/
+  x = 0;// CHECK:  [[@LINE]]| 1|*/ x = 0;
+// CHECK:  [[@LINE]]|  |
+  /* comment */ // CHECK:  [[@LINE]]|  |/* comment */
+  // comment // CHECK:  [[@LINE]]|  |// comment
+  /* comment */ // CHECK:  [[@LINE]]|  |/* comment */
+  z =   // CHECK:  [[@LINE]]| 1|z =
+  x // comment   // CHECK:  [[@LINE]]| 1|x // comment
+  // comment // CHECK:  [[@LINE]]|  |// comment
+  +  /*   // CHECK:  [[@LINE]]| 1|+ /*
+comment// CHECK:  [[@LINE]]|  |comment
+*/
+ // CHECK:  [[@LINE]]|  |*/
+ /* // CHECK:  [[@LINE]]|  |/*
+comment// CHECK:  [[@LINE]]|  |comment
+*/
+  y; // CHECK:  [[@LINE]]| 1|*/y;
+ // CHECK:  [[@LINE]]|  |
+// Comments inside directives. // CHECK:  [[@LINE]]|  |// Comments inside directives.
+#if 0 //comment// CHECK:  [[@LINE]]|  |#if 0 //comment
+/* comment */ x = 0;   // CHECK:  [[@LINE]]|  |/* comment */ x = 0;
+y = 0; /* comment */   // CHECK:  [[@LINE]]|  |y = 0; /* comment */
+z = 0; // comment  // CHECK:  [[@LINE]]|  |z = 0; // comment
+// comment // CHECK:  [[@LINE]]|  |// comment
+   // CHECK:  [[@LINE]]|  |
+x = 0; /*  // CHECK:  [[@LINE]]|  |x = 0; /*
+comment//

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

2020-07-16 Thread Zequan Wu via Phabricator via cfe-commits

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 if there is `SkippedRegion` in 
> > the same line and then mark the decl range as `CodeRegion`. For the 
> > example, we will have 3 more `CodeRegion` which covers the ranges of `int x 
> > = 0`, `int y = 0` and `int z = 0`.
>
> I don't think that will work well, because a decl can span multiple lines 
> (which in turn can include comments).
>
> The key issue seems to be that -- even having SourceRanges for all comments 
> -- we don't know whether a line has non-whitespace, non-comment tokens. If it 
> does, the line should have an execution count (and we don't even need to 
> bother with emitting a skipped region covering the line). It sounds like we 
> need a different hook in the preprocessor that reports SourceRanges for lines 
> with no non-comment, non-whitespace tokens. Practically, I think this can be 
> wired up just like a CommentHandler (maybe it could be called 
> WhitespaceHandler?). One side-benefit of introducing a new hook like this is 
> that coverage will handle whitespace-only lines the same way as comment-only 
> lines.
>
> There's probably more than one way to approach this. One thing to be aware 
> of: llvm's LineCoverageStats class is the one responsible for determining 
> whether a line is "mapped" (i.e. whether it has an execution count). It 
> assumes that when a line begins with a skipped region, the subsequent portion 
> of the line must also be skipped. If we only emit skipped regions for fully 
> whitespace-like lines, that's fine. If not, depending on how C-style comments 
> are handled, that may need updating.


Thanks for the reply. I will work on that.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-16 Thread Vedant Kumar via Phabricator via cfe-commits

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 `CodeRegion`. For the example, we 
> will have 3 more `CodeRegion` which covers the ranges of `int x = 0`, `int y 
> = 0` and `int z = 0`.

I don't think that will work well, because a decl can span multiple lines 
(which in turn can include comments).

The key issue seems to be that -- even having SourceRanges for all comments -- 
we don't know whether a line has non-whitespace, non-comment tokens. If it 
does, the line should have an execution count (and we don't even need to bother 
with emitting a skipped region covering the line). It sounds like we need a 
different hook in the preprocessor that reports SourceRanges for lines with no 
non-comment, non-whitespace tokens. Practically, I think this can be wired up 
just like a CommentHandler (maybe it could be called WhitespaceHandler?). One 
side-benefit of introducing a new hook like this is that coverage will handle 
whitespace-only lines the same way as comment-only lines.

There's probably more than one way to approach this. One thing to be aware of: 
llvm's LineCoverageStats class is the one responsible for determining whether a 
line is "mapped" (i.e. whether it has an execution count). It assumes that when 
a line begins with a skipped region, the subsequent portion of the line must 
also be skipped. If we only emit skipped regions for fully whitespace-like 
lines, that's fine. If not, depending on how C-style comments are handled, that 
may need updating.

Repository:
  rG LLVM Github Monorepo

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

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

2020-07-16 Thread Zequan Wu via Phabricator via cfe-commits

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)? 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 the end of a line`
> > - `// comment spanning entire line`
> >
> >   In all but the last case, llvm-cov should report an execution count for 
> > the line. Testing multi-line variations of those cases would also be 
> > helpful.
>
>
> llvm-cov doesn't report the execution count for the first 3 liens... I will 
> try to fix that.


The problem is that simply marking comment regions as `SkippedRegion` doesn't 
give enough information for `llvm-cov` about if there is code area before or 
after comments in the same line.

  $ clang++ -fcoverage-mapping -fprofile-instr-generate /tmp/a.cpp -Xclang 
-dump-coverage-mapping && ./a.out && llvm-profdata merge -sparse 
default.profraw -o a.profdata && llvm-cov show ./a.out 
-instr-profile=a.profdata -debug-only="coverage-mapping"
  main:
File 0, 1:12 -> 7:2 = #0
Skipped,File 0, 2:3 -> 2:39 = 0
Skipped,File 0, 3:15 -> 3:49 = 0
Skipped,File 0, 4:15 -> 4:46 = 0
Skipped,File 0, 5:3 -> 5:34 = 0
  Counter in file 0 1:12 -> 7:2, #0
  Counter in file 0 2:3 -> 2:39, 0
  Counter in file 0 3:15 -> 3:49, 0
  Counter in file 0 4:15 -> 4:46, 0
  Counter in file 0 5:3 -> 5:34, 0
  Emitting segments for file: /tmp/a.cpp
  Combined regions:
1:12 -> 7:2 (count=1)
2:3 -> 2:39 (count=0)
3:15 -> 3:49 (count=0)
4:15 -> 4:46 (count=0)
5:3 -> 5:34 (count=0)
  Segment at 1:12 (count = 1), RegionEntry
  Segment at 2:3 (count = 0), RegionEntry, Skipped
  Segment at 2:39 (count = 1)
  Segment at 3:15 (count = 0), RegionEntry, Skipped
  Segment at 3:49 (count = 1)
  Segment at 4:15 (count = 0), RegionEntry, Skipped
  Segment at 4:46 (count = 1)
  Segment at 5:3 (count = 0), RegionEntry, Skipped
  Segment at 5:34 (count = 1)
  Segment at 7:2 (count = 0), Skipped
  1|  1|int main() {
  2|   |  /* comment at the start of a line */ int x = 0;
  3|   |  int y = 0;  /* comment at the end of a line */
  4|   |  int z = 0;  // comment at the end of a line
  5|   |  // comment spanning entire line
  6|  1|  return 0;
  7|  1|}
  8|   |

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 `CodeRegion`. For the example, we 
will have 3 more `CodeRegion` which covers the ranges of `int x = 0`, `int y = 
0` and `int z = 0`.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-14 Thread Zequan Wu via Phabricator via cfe-commits

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 of a line */ expr;`
> - `expr; /* comment at the end of a line */`
> - `expr; // comment at the end of a line`
> - `// comment spanning entire line`
>
>   In all but the last case, llvm-cov should report an execution count for the 
> line. Testing multi-line variations of those cases would also be helpful.


llvm-cov doesn't report the execution count for the first 3 liens... I will try 
to fix that.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-14 Thread Zequan Wu via Phabricator via cfe-commits

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:
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/test/CoverageMapping/break.c
  clang/test/CoverageMapping/builtinmacro.c
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/comment-in-macro.c
  clang/test/CoverageMapping/continue.c
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/if.cpp
  clang/test/CoverageMapping/includehell.cpp
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/logical.cpp
  clang/test/CoverageMapping/loops.cpp
  clang/test/CoverageMapping/macro-expressions.cpp
  clang/test/CoverageMapping/macroparams2.c
  clang/test/CoverageMapping/macros.c
  clang/test/CoverageMapping/macroscopes.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/objc.m
  clang/test/CoverageMapping/pr32679.cpp
  clang/test/CoverageMapping/preprocessor.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/test.c
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  clang/test/CoverageMapping/while.c
  clang/test/lit.cfg.py

Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -89,6 +89,11 @@
 ('%hmaptool', "'%s' %s" % (config.python_executable,
  os.path.join(config.clang_tools_dir, 'hmaptool'
 
+# Strip C++ comments "//"" from tests
+config.substitutions.append(
+('%strip_comments', "sed 's/[ \t]*\/\/.*//' %s > %T/%basename_t")
+)
+
 # Plugins (loadable modules)
 if config.has_plugins and config.llvm_plugin_ext:
 config.available_features.add('plugins')
Index: clang/test/CoverageMapping/while.c
===
--- clang/test/CoverageMapping/while.c
+++ clang/test/CoverageMapping/while.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name loops.cpp %s | FileCheck %s
+// RUN: %strip_comments && %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name loops.cpp %T/%basename_t | FileCheck %s
 
 // CHECK: main
 int main() {// CHECK-NEXT: File 0, [[@LINE]]:12 -> [[@LINE+8]]:2 = #0
Index: clang/test/CoverageMapping/unreachable-macro.c
===
--- clang/test/CoverageMapping/unreachable-macro.c
+++ clang/test/CoverageMapping/unreachable-macro.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s | FileCheck %s
+// RUN: %strip_comments && %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %T/%basename_t | FileCheck %s
 
 #define WHILE while (0) {}
 
Index: clang/test/CoverageMapping/trycatch.cpp
===
--- clang/test/CoverageMapping/trycatch.cpp
+++ clang/test/CoverageMapping/trycatch.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fexceptions -fcxx-exceptions -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name trycatch.cpp %s | FileCheck %s
+// RUN: %strip_comments && %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fexceptions -fcxx-exceptions -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name trycatch.cpp %T/%basename_t | FileCheck %s
 
 class Error {
 };
Index: clang/test/CoverageMapping/test.c
===
--- clang/test/CoverageMapping/test.c
+++ clang/test/CoverageMapping/test.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name test.c %s | FileCheck %s
+// RUN: %strip_comments && %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name test.c %T/%basename_t | FileCheck %s
 
 void bar();
 static void static_func();
Index: clang/test/CoverageMapping/switchmacro.c
===
--- clang/test/CoverageMapping/switchmacro.c
+++ clang/test/CoverageMapping/switchmacro.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only

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

2020-07-14 Thread Zequan Wu via Phabricator via cfe-commits

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

Files:
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/test/CoverageMapping/break.c
  clang/test/CoverageMapping/builtinmacro.c
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/comment-in-macro.c
  clang/test/CoverageMapping/continue.c
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/if.cpp
  clang/test/CoverageMapping/includehell.cpp
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/logical.cpp
  clang/test/CoverageMapping/loops.cpp
  clang/test/CoverageMapping/macro-expressions.cpp
  clang/test/CoverageMapping/macroparams2.c
  clang/test/CoverageMapping/macros.c
  clang/test/CoverageMapping/macroscopes.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/objc.m
  clang/test/CoverageMapping/pr32679.cpp
  clang/test/CoverageMapping/preprocessor.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/test.c
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  clang/test/CoverageMapping/while.c
  clang/test/lit.cfg.py

Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -89,6 +89,11 @@
 ('%hmaptool', "'%s' %s" % (config.python_executable,
  os.path.join(config.clang_tools_dir, 'hmaptool'
 
+# Strip C++ comments "//"" from tests
+config.substitutions.append(
+('%strip_comments', "sed 's/\/\/.*//' %s > %T/%basename_t")
+)
+
 # Plugins (loadable modules)
 if config.has_plugins and config.llvm_plugin_ext:
 config.available_features.add('plugins')
Index: clang/test/CoverageMapping/while.c
===
--- clang/test/CoverageMapping/while.c
+++ clang/test/CoverageMapping/while.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name loops.cpp %s | FileCheck %s
+// RUN: %strip_comments && %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name loops.cpp %T/%basename_t | FileCheck %s
 
 // CHECK: main
 int main() {// CHECK-NEXT: File 0, [[@LINE]]:12 -> [[@LINE+8]]:2 = #0
Index: clang/test/CoverageMapping/unreachable-macro.c
===
--- clang/test/CoverageMapping/unreachable-macro.c
+++ clang/test/CoverageMapping/unreachable-macro.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s | FileCheck %s
+// RUN: %strip_comments && %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %T/%basename_t | FileCheck %s
 
 #define WHILE while (0) {}
 
Index: clang/test/CoverageMapping/trycatch.cpp
===
--- clang/test/CoverageMapping/trycatch.cpp
+++ clang/test/CoverageMapping/trycatch.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fexceptions -fcxx-exceptions -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name trycatch.cpp %s | FileCheck %s
+// RUN: %strip_comments && %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fexceptions -fcxx-exceptions -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name trycatch.cpp %T/%basename_t | FileCheck %s
 
 class Error {
 };
Index: clang/test/CoverageMapping/test.c
===
--- clang/test/CoverageMapping/test.c
+++ clang/test/CoverageMapping/test.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name test.c %s | FileCheck %s
+// RUN: %strip_comments && %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name test.c %T/%basename_t | FileCheck %s
 
 void bar();
 static void static_func();
Index: clang/test/CoverageMapping/switchmacro.c
===
--- clang/test/CoverageMapping/switchmacro.c
+++ clang/test/CoverageMapping/switchmacro.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only

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

2020-07-14 Thread Vedant Kumar via Phabricator via cfe-commits

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 the end of a line`
- `// comment spanning entire line`

In all but the last case, llvm-cov should report an execution count for the 
line.




Comment at: clang/test/CoverageMapping/break.c:1
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping 
-dump-coverage-mapping -emit-llvm-only -main-file-name break.c %s | FileCheck %s
+// RUN: sed 's/\/\/.*//' %s > %T/%basename_t && %clang_cc1 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -main-file-name break.c %T/%basename_t | FileCheck %s
 

Could you introduce a lit substitution in clang/test/lit.cfg.py for this (say, 
'%strip_comments'?), so that there's just one copy of the sed command?


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-14 Thread Zequan Wu via Phabricator via cfe-commits

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:
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/test/CoverageMapping/break.c
  clang/test/CoverageMapping/builtinmacro.c
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/comment-in-macro.c
  clang/test/CoverageMapping/continue.c
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/if.cpp
  clang/test/CoverageMapping/includehell.cpp
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/logical.cpp
  clang/test/CoverageMapping/loops.cpp
  clang/test/CoverageMapping/macro-expressions.cpp
  clang/test/CoverageMapping/macroparams2.c
  clang/test/CoverageMapping/macros.c
  clang/test/CoverageMapping/macroscopes.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/objc.m
  clang/test/CoverageMapping/pr32679.cpp
  clang/test/CoverageMapping/preprocessor.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/test.c
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  clang/test/CoverageMapping/while.c

Index: clang/test/CoverageMapping/while.c
===
--- clang/test/CoverageMapping/while.c
+++ clang/test/CoverageMapping/while.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name loops.cpp %s | FileCheck %s
+// RUN: sed 's/\/\/.*//' %s > %T/%basename_t && %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name loops.cpp %T/%basename_t | FileCheck %s
 
 // CHECK: main
 int main() {// CHECK-NEXT: File 0, [[@LINE]]:12 -> [[@LINE+8]]:2 = #0
Index: clang/test/CoverageMapping/unreachable-macro.c
===
--- clang/test/CoverageMapping/unreachable-macro.c
+++ clang/test/CoverageMapping/unreachable-macro.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s | FileCheck %s
+// RUN: sed 's/\/\/.*//' %s > %T/%basename_t && %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %T/%basename_t | FileCheck %s
 
 #define WHILE while (0) {}
 
Index: clang/test/CoverageMapping/trycatch.cpp
===
--- clang/test/CoverageMapping/trycatch.cpp
+++ clang/test/CoverageMapping/trycatch.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fexceptions -fcxx-exceptions -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name trycatch.cpp %s | FileCheck %s
+// RUN: sed 's/\/\/.*//' %s > %T/%basename_t && %clang_cc1 -triple %itanium_abi_triple -std=c++11 -fexceptions -fcxx-exceptions -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name trycatch.cpp %T/%basename_t | FileCheck %s
 
 class Error {
 };
Index: clang/test/CoverageMapping/test.c
===
--- clang/test/CoverageMapping/test.c
+++ clang/test/CoverageMapping/test.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name test.c %s | FileCheck %s
+// RUN: sed 's/\/\/.*//' %s > %T/%basename_t && %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name test.c %T/%basename_t | FileCheck %s
 
 void bar();
 static void static_func();
Index: clang/test/CoverageMapping/switchmacro.c
===
--- clang/test/CoverageMapping/switchmacro.c
+++ clang/test/CoverageMapping/switchmacro.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name switchmacro.c %s | FileCheck %s
+// RUN: sed 's/\/\/.*//' %s > %T/%basename_t && %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name switchmacro.c %T/%basename_t | FileCheck %s
 
 #define FOO(x) (void)x
 
Index: clang/test/CoverageMapping/switch.cpp
===
--- clang/test/CoverageMapping/switch.cpp
+++ clang/test/CoverageMapping/switch.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fprofile-instrument=clang

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

2020-07-14 Thread Hans Wennborg via Phabricator via cfe-commits

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 really part of the test, maybe we 
could do something to exclude them, like running 'sed' before compiling, or 
wrapping them in #if 0 blocks or something.

But figuring that out seems less important than figuring out how to implement 
the feature, and I think the current approach looks promising.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-13 Thread Zequan Wu via Phabricator via cfe-commits

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 
> 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|  }
>
>
> The loop region covers lines 1-3. If we skip the comment range on line 2, 
> does an execution count from the loop region still get picked up? I'd expect 
> it to. It's possible that we need more information from the preprocessor 
> about whether the line is fully comment/whitespace-only.


Here is what I got:

  $ clang -fcoverage-mapping -fprofile-instr-generate /tmp/a.c -Xclang 
-dump-coverage-mapping && ./a.out && llvm-profdata merge -sparse 
default.profraw -o a.profdata && llvm-cov show ./a.out -instr-profile=a.profdata
  main:
File 0, 1:12 -> 6:2 = #0
File 0, 2:19 -> 2:25 = (#0 + #1)
File 0, 2:27 -> 2:30 = #1
Gap,File 0, 2:31 -> 2:32 = #1
File 0, 2:32 -> 4:4 = #1
Skipped,File 0, 3:5 -> 3:15 = 0
  1|  1|int main() {
  2| 11|  for (int i = 0; i < 10; i++) {
  3|   |// comment
  4| 10|  }
  5|  1|  return 0;
  6|  1|}

For those failed test cases, they are caused by extra `Skipped, File ...` lines 
which invalidate some `CHECK-NEXT`. 
I could either change `CHECK-NEXT` to `CHECK` or moving those checks inside 
function to above the function, since comments outside functions will not be 
tracked.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-13 Thread Vedant Kumar via Phabricator via cfe-commits

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|  }

The loop region covers lines 1-3. If we skip the comment range on line 2, does 
an execution count from the loop region still get picked up? I'd expect it to. 
It's possible that we need more information from the preprocessor about whether 
the line is fully comment/whitespace-only.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-13 Thread Zequan Wu via Phabricator via cfe-commits

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:
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h


Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -16,6 +16,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Support/raw_ostream.h"
@@ -32,12 +33,14 @@
 /// Stores additional source code information like skipped ranges which
 /// is required by the coverage mapping generator and is obtained from
 /// the preprocessor.
-class CoverageSourceInfo : public PPCallbacks {
+class CoverageSourceInfo : public PPCallbacks, public CommentHandler {
   std::vector SkippedRanges;
 public:
   ArrayRef getSkippedRanges() const { return SkippedRanges; }
 
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
+
+  bool HandleComment(Preprocessor , SourceRange Range) override;
 };
 
 namespace CodeGen {
@@ -66,6 +69,8 @@
  uint64_t FilenamesRef);
 
 public:
+  static CoverageSourceInfo *setUpCoverageCallbacks(Preprocessor );
+
   CoverageMappingModuleGen(CodeGenModule , CoverageSourceInfo )
   : CGM(CGM), SourceInfo(SourceInfo) {}
 
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -35,10 +35,23 @@
 using namespace CodeGen;
 using namespace llvm::coverage;
 
+CoverageSourceInfo *
+CoverageMappingModuleGen::setUpCoverageCallbacks(Preprocessor ) {
+  CoverageSourceInfo *CoverageInfo = new CoverageSourceInfo;
+  PP.addPPCallbacks(std::unique_ptr(CoverageInfo));
+  PP.addCommentHandler(CoverageInfo);
+  return CoverageInfo;
+}
+
 void CoverageSourceInfo::SourceRangeSkipped(SourceRange Range, SourceLocation) 
{
   SkippedRanges.push_back(Range);
 }
 
+bool CoverageSourceInfo::HandleComment(Preprocessor &, SourceRange Range) {
+  SkippedRanges.push_back(Range);
+  return false;
+}
+
 namespace {
 
 /// A region of source code that can be mapped to a counter.
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -990,11 +990,9 @@
 
   CoverageSourceInfo *CoverageInfo = nullptr;
   // Add the preprocessor callback only when the coverage mapping is generated.
-  if (CI.getCodeGenOpts().CoverageMapping) {
-CoverageInfo = new CoverageSourceInfo;
-CI.getPreprocessor().addPPCallbacks(
-
std::unique_ptr(CoverageInfo));
-  }
+  if (CI.getCodeGenOpts().CoverageMapping)
+CoverageInfo = CodeGen::CoverageMappingModuleGen::setUpCoverageCallbacks(
+CI.getPreprocessor());
 
   std::unique_ptr Result(new BackendConsumer(
   BA, CI.getDiagnostics(), CI.getHeaderSearchOpts(),


Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -16,6 +16,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Support/raw_ostream.h"
@@ -32,12 +33,14 @@
 /// Stores additional source code information like skipped ranges which
 /// is required by the coverage mapping generator and is obtained from
 /// the preprocessor.
-class CoverageSourceInfo : public PPCallbacks {
+class CoverageSourceInfo : public PPCallbacks, public CommentHandler {
   std::vector SkippedRanges;
 public:
   ArrayRef getSkippedRanges() const { return SkippedRanges; }
 
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
+
+  bool HandleComment(Preprocessor , SourceRange Range) override;
 };
 
 namespace CodeGen {
@@ -66,6 +69,8 @@
  uint64_t FilenamesRef);
 
 public:
+  static CoverageSourceInfo *setUpCoverageCallbacks(Preprocessor );
+
   CoverageMappingModuleGen(CodeGenModule , CoverageSourceInfo )
   : CGM(CGM), SourceInfo(SourceInfo) {}
 
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -35,10 +35,23 @@
 using namespace

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

2020-07-13 Thread Zequan Wu via Phabricator via cfe-commits

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
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  default.profraw


Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -16,6 +16,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Support/raw_ostream.h"
@@ -32,12 +33,14 @@
 /// Stores additional source code information like skipped ranges which
 /// is required by the coverage mapping generator and is obtained from
 /// the preprocessor.
-class CoverageSourceInfo : public PPCallbacks {
+class CoverageSourceInfo : public PPCallbacks, public CommentHandler {
   std::vector SkippedRanges;
 public:
   ArrayRef getSkippedRanges() const { return SkippedRanges; }
 
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
+
+  bool HandleComment(Preprocessor , SourceRange Range) override;
 };
 
 namespace CodeGen {
@@ -66,6 +69,8 @@
  uint64_t FilenamesRef);
 
 public:
+  static CoverageSourceInfo *setUpCoverageCallbacks(Preprocessor );
+
   CoverageMappingModuleGen(CodeGenModule , CoverageSourceInfo )
   : CGM(CGM), SourceInfo(SourceInfo) {}
 
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -35,10 +35,23 @@
 using namespace CodeGen;
 using namespace llvm::coverage;
 
+CoverageSourceInfo *
+CoverageMappingModuleGen::setUpCoverageCallbacks(Preprocessor ) {
+  CoverageSourceInfo *CoverageInfo = new CoverageSourceInfo;
+  PP.addPPCallbacks(std::unique_ptr(CoverageInfo));
+  PP.addCommentHandler(CoverageInfo);
+  return CoverageInfo;
+}
+
 void CoverageSourceInfo::SourceRangeSkipped(SourceRange Range, SourceLocation) 
{
   SkippedRanges.push_back(Range);
 }
 
+bool CoverageSourceInfo::HandleComment(Preprocessor &, SourceRange Range) {
+  SkippedRanges.push_back(Range);
+  return false;
+}
+
 namespace {
 
 /// A region of source code that can be mapped to a counter.
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -990,11 +990,9 @@
 
   CoverageSourceInfo *CoverageInfo = nullptr;
   // Add the preprocessor callback only when the coverage mapping is generated.
-  if (CI.getCodeGenOpts().CoverageMapping) {
-CoverageInfo = new CoverageSourceInfo;
-CI.getPreprocessor().addPPCallbacks(
-
std::unique_ptr(CoverageInfo));
-  }
+  if (CI.getCodeGenOpts().CoverageMapping)
+CoverageInfo = CodeGen::CoverageMappingModuleGen::setUpCoverageCallbacks(
+CI.getPreprocessor());
 
   std::unique_ptr Result(new BackendConsumer(
   BA, CI.getDiagnostics(), CI.getHeaderSearchOpts(),


Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -16,6 +16,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Support/raw_ostream.h"
@@ -32,12 +33,14 @@
 /// Stores additional source code information like skipped ranges which
 /// is required by the coverage mapping generator and is obtained from
 /// the preprocessor.
-class CoverageSourceInfo : public PPCallbacks {
+class CoverageSourceInfo : public PPCallbacks, public CommentHandler {
   std::vector SkippedRanges;
 public:
   ArrayRef getSkippedRanges() const { return SkippedRanges; }
 
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
+
+  bool HandleComment(Preprocessor , SourceRange Range) override;
 };
 
 namespace CodeGen {
@@ -66,6 +69,8 @@
  uint64_t FilenamesRef);
 
 public:
+  static CoverageSourceInfo *setUpCoverageCallbacks(Preprocessor );
+
   CoverageMappingModuleGen(CodeGenModule , CoverageSourceInfo )
   : CGM(CGM), SourceInfo(SourceInfo) {}
 
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -35,10 +35,23 @@
 using

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

2020-07-13 Thread Zequan Wu via Phabricator via cfe-commits

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 wrote:
> zequanwu wrote:
> > vsk wrote:
> > > This seems like a lot of complexity to add to handle a narrow case. Is it 
> > > necessary to merge skipped regions early in the process? Note that llvm's 
> > > SegmentBuilder takes care of merging regions at the very end.
> > Merging at here is to avoid duplicate output for option 
> > `-dump-coverage-mapping`. Like the following example to avoid `Skipped,File 
> > 0, 2:3 -> 4:9 = 0` and `Skipped,File 0, 2:15 -> 2:25 = 0` to be outputted 
> > at the same time. They should be merged into 1 region `Skipped,File 0, 2:3 
> > -> 4:9 = 0`
> > ```
> > File 0, 1:12 -> 6:2 = #0
> >  Skipped,File 0, 2:3 -> 4:9 = 0
> >  Skipped,File 0, 2:15 -> 2:25 = 0
> >1|  1|int main() {
> >2|   |  #ifdef TEST // comment
> >3|   |  int x = 1;
> >4|   |  #endif
> >5|  1|  return 0;
> >6|  1|}
> > ```
> > So, we need to do it early.
> I see that there are duplicate 'Skipped' regions emitted in this case, but 
> I'm not sure what problems this causes. For testing, we could check that both 
> skipped regions are emitted, or perhaps only check that the outermost skipped 
> region is emitted and ignore others. Is there some other kind of hard error 
> (like an assert) caused by having nested skipped regions? If not, perhaps 
> it's not worth it to merge them.
No, it doesn't cause any error.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-13 Thread Zequan Wu via Phabricator via cfe-commits

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

Files:
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Parse/Parser.cpp

Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -34,6 +34,7 @@
   explicit ActionCommentHandler(Sema ) : S(S) { }
 
   bool HandleComment(Preprocessor , SourceRange Comment) override {
+PP.getPPCallbacks()->CommentSkipped(Comment);
 S.ActOnComment(Comment);
 return false;
   }
Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -16,6 +16,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Support/raw_ostream.h"
@@ -32,12 +33,14 @@
 /// Stores additional source code information like skipped ranges which
 /// is required by the coverage mapping generator and is obtained from
 /// the preprocessor.
-class CoverageSourceInfo : public PPCallbacks {
+class CoverageSourceInfo : public PPCallbacks, public CommentHandler {
   std::vector SkippedRanges;
 public:
   ArrayRef getSkippedRanges() const { return SkippedRanges; }
 
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
+
+  bool HandleComment(Preprocessor , SourceRange Range) override;
 };
 
 namespace CodeGen {
@@ -66,6 +69,8 @@
  uint64_t FilenamesRef);
 
 public:
+  static CoverageSourceInfo *setUpCoverageCallbacks(Preprocessor );
+
   CoverageMappingModuleGen(CodeGenModule , CoverageSourceInfo )
   : CGM(CGM), SourceInfo(SourceInfo) {}
 
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -35,10 +35,23 @@
 using namespace CodeGen;
 using namespace llvm::coverage;
 
+CoverageSourceInfo *
+CoverageMappingModuleGen::setUpCoverageCallbacks(Preprocessor ) {
+  CoverageSourceInfo *CoverageInfo = new CoverageSourceInfo;
+  PP.addPPCallbacks(std::unique_ptr(CoverageInfo));
+  PP.addCommentHandler(CoverageInfo);
+  return CoverageInfo;
+}
+
 void CoverageSourceInfo::SourceRangeSkipped(SourceRange Range, SourceLocation) {
   SkippedRanges.push_back(Range);
 }
 
+bool CoverageSourceInfo::HandleComment(Preprocessor &, SourceRange Range) {
+  SkippedRanges.push_back(Range);
+  return false;
+}
+
 namespace {
 
 /// A region of source code that can be mapped to a counter.
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -990,11 +990,9 @@
 
   CoverageSourceInfo *CoverageInfo = nullptr;
   // Add the preprocessor callback only when the coverage mapping is generated.
-  if (CI.getCodeGenOpts().CoverageMapping) {
-CoverageInfo = new CoverageSourceInfo;
-CI.getPreprocessor().addPPCallbacks(
-std::unique_ptr(CoverageInfo));
-  }
+  if (CI.getCodeGenOpts().CoverageMapping)
+CoverageInfo = CodeGen::CoverageMappingModuleGen::setUpCoverageCallbacks(
+CI.getPreprocessor());
 
   std::unique_ptr Result(new BackendConsumer(
   BA, CI.getDiagnostics(), CI.getHeaderSearchOpts(),
Index: clang/include/clang/Lex/PPCallbacks.h
===
--- clang/include/clang/Lex/PPCallbacks.h
+++ clang/include/clang/Lex/PPCallbacks.h
@@ -319,6 +319,10 @@
   virtual void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) {
   }
 
+  /// Hood called when the source range is comment, which should be skipped.
+  /// \param Range The SourceRange that is comment.
+  virtual void CommentSkipped(SourceRange Range) {}
+
   enum ConditionValueKind {
 CVK_NotEvaluated, CVK_False, CVK_True
   };
@@ -565,6 +569,11 @@
 Second->SourceRangeSkipped(Range, EndifLoc);
   }
 
+  void CommentSkipped(SourceRange Range) override {
+First->CommentSkipped(Range);
+Second->CommentSkipped(Range);
+  }
+
   /// Hook called whenever an \#if is seen.
   void If(SourceLocation Loc, SourceRange ConditionRange,
   ConditionValueKind ConditionValue) override {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

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

2020-07-13 Thread Vedant Kumar via Phabricator via cfe-commits

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 
> > 'CoverageMappingGen::setupPreprocessorCallbacks(Preprocessor &)' and call 
> > that from CodeGenAction::CreateASTConsumer.
>
>
> Sorry, I don't quite understand this. I am already using 
> `ActionCommentHandler` to handle this one inside `HandleComment`.


ActionCommentHandler seems a bit specific to Sema. This is what I had in mind: 
https://github.com/vedantk/llvm-project/commit/5101dd5dcd92742805069dba6b3d6d6846ef3755.




Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:152
+static MutableArrayRef
+mergeSkippedRegions(MutableArrayRef MappingRegions) {
+  SmallVector MergedRegions;

zequanwu wrote:
> vsk wrote:
> > This seems like a lot of complexity to add to handle a narrow case. Is it 
> > necessary to merge skipped regions early in the process? Note that llvm's 
> > SegmentBuilder takes care of merging regions at the very end.
> Merging at here is to avoid duplicate output for option 
> `-dump-coverage-mapping`. Like the following example to avoid `Skipped,File 
> 0, 2:3 -> 4:9 = 0` and `Skipped,File 0, 2:15 -> 2:25 = 0` to be outputted at 
> the same time. They should be merged into 1 region `Skipped,File 0, 2:3 -> 
> 4:9 = 0`
> ```
> File 0, 1:12 -> 6:2 = #0
>  Skipped,File 0, 2:3 -> 4:9 = 0
>  Skipped,File 0, 2:15 -> 2:25 = 0
>1|  1|int main() {
>2|   |  #ifdef TEST // comment
>3|   |  int x = 1;
>4|   |  #endif
>5|  1|  return 0;
>6|  1|}
> ```
> So, we need to do it early.
I see that there are duplicate 'Skipped' regions emitted in this case, but I'm 
not sure what problems this causes. For testing, we could check that both 
skipped regions are emitted, or perhaps only check that the outermost skipped 
region is emitted and ignore others. Is there some other kind of hard error 
(like an assert) caused by having nested skipped regions? If not, perhaps it's 
not worth it to merge them.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-13 Thread Zequan Wu via Phabricator via cfe-commits

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 
> 'CoverageMappingGen::setupPreprocessorCallbacks(Preprocessor &)' and call 
> that from CodeGenAction::CreateASTConsumer.


Sorry, I don't quite understand this. I am already using `ActionCommentHandler` 
to handle this one inside `HandleComment`.




Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:152
+static MutableArrayRef
+mergeSkippedRegions(MutableArrayRef MappingRegions) {
+  SmallVector MergedRegions;

vsk wrote:
> This seems like a lot of complexity to add to handle a narrow case. Is it 
> necessary to merge skipped regions early in the process? Note that llvm's 
> SegmentBuilder takes care of merging regions at the very end.
Merging at here is to avoid duplicate output for option 
`-dump-coverage-mapping`. Like the following example to avoid `Skipped,File 0, 
2:3 -> 4:9 = 0` and `Skipped,File 0, 2:15 -> 2:25 = 0` to be outputted at the 
same time. They should be merged into 1 region `Skipped,File 0, 2:3 -> 4:9 = 0`
```
File 0, 1:12 -> 6:2 = #0
 Skipped,File 0, 2:3 -> 4:9 = 0
 Skipped,File 0, 2:15 -> 2:25 = 0
   1|  1|int main() {
   2|   |  #ifdef TEST // comment
   3|   |  int x = 1;
   4|   |  #endif
   5|  1|  return 0;
   6|  1|}
```
So, we need to do it early.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-13 Thread Vedant Kumar via Phabricator via cfe-commits

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 
'CoverageMappingGen::setupPreprocessorCallbacks(Preprocessor &)' and call that 
from CodeGenAction::CreateASTConsumer.




Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:152
+static MutableArrayRef
+mergeSkippedRegions(MutableArrayRef MappingRegions) {
+  SmallVector MergedRegions;

This seems like a lot of complexity to add to handle a narrow case. Is it 
necessary to merge skipped regions early in the process? Note that llvm's 
SegmentBuilder takes care of merging regions at the very end.



Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:183
+
+  return MergedRegions;
+}

This is stack use after free, right? This needs to release ownership of the 
'MergedRegions' container.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-13 Thread Zequan Wu via Phabricator via cfe-commits

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/

https://reviews.llvm.org/D83592

Files:
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Parse/Parser.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
@@ -148,6 +148,41 @@
   encodeULEB128(encodeCounter(Expressions, C), OS);
 }
 
+static MutableArrayRef
+mergeSkippedRegions(MutableArrayRef MappingRegions) {
+  SmallVector MergedRegions;
+  for (size_t I = 0; I < MappingRegions.size() - 1; I++) {
+CounterMappingRegion First = MappingRegions[I];
+CounterMappingRegion Second = MappingRegions[I+1];
+if (First.FileID != Second.FileID ||
+First.Kind != CounterMappingRegion::SkippedRegion ||
+First.Kind != Second.Kind) {
+  MergedRegions.push_back(First);
+  continue;
+}
+
+if (First.LineEnd > Second.LineEnd ||
+(First.LineEnd == Second.LineEnd &&
+ First.ColumnEnd >= Second.ColumnEnd)) {
+  // When second region is inside first region, discard second region.
+  MergedRegions.push_back(First);
+  I++;
+} else if (Second.LineStart >= First.LineStart &&
+   Second.LineStart <= First.LineEnd) {
+  // When the start location of second region is inside first region and the
+  // end location is outside first region.
+  MergedRegions.push_back(CounterMappingRegion::makeSkipped(
+  First.FileID, First.LineStart, First.ColumnStart, Second.LineEnd,
+  Second.ColumnEnd));
+} else {
+  // When there is no overlapping.
+  MergedRegions.push_back(First);
+}
+  }
+
+  return MergedRegions;
+}
+
 void CoverageMappingWriter::write(raw_ostream ) {
   // Check that we don't have any bogus regions.
   assert(all_of(MappingRegions,
@@ -167,6 +202,9 @@
 return LHS.Kind < RHS.Kind;
   });
 
+  // Merge overlapped regions.
+  MappingRegions = mergeSkippedRegions(MappingRegions);
+
   // Write out the fileid -> filename mapping.
   encodeULEB128(VirtualFileMapping.size(), OS);
   for (const auto  : VirtualFileMapping)
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -34,6 +34,7 @@
   explicit ActionCommentHandler(Sema ) : S(S) { }
 
   bool HandleComment(Preprocessor , SourceRange Comment) override {
+PP.getPPCallbacks()->CommentSkipped(Comment);
 S.ActOnComment(Comment);
 return false;
   }
Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -38,6 +38,8 @@
   ArrayRef getSkippedRanges() const { return SkippedRanges; }
 
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
+
+  void CommentSkipped(SourceRange Range) override;
 };
 
 namespace CodeGen {
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -39,6 +39,10 @@
   SkippedRanges.push_back(Range);
 }
 
+void CoverageSourceInfo::CommentSkipped(SourceRange Range) {
+  SkippedRanges.push_back(Range);
+}
+
 namespace {
 
 /// A region of source code that can be mapped to a counter.
Index: clang/include/clang/Lex/PPCallbacks.h
===
--- clang/include/clang/Lex/PPCallbacks.h
+++ clang/include/clang/Lex/PPCallbacks.h
@@ -319,6 +319,10 @@
   virtual void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) {
   }
 
+  /// Hood called when the source range is comment, which should be skipped.
+  /// \param Range The SourceRange that is comment.
+  virtual void CommentSkipped(SourceRange Range) {}
+
   enum ConditionValueKind {
 CVK_NotEvaluated, CVK_False, CVK_True
   };
@@ -565,6 +569,11 @@
 Second->SourceRangeSkipped(Range, EndifLoc);
   }
 
+  void CommentSkipped(SourceRange Range) override {
+First->CommentSkipped(Range);
+Second->CommentSkipped(Range);
+  }
+
   /// Hook called whenever an \#if is seen.
   void If(SourceLocation Loc, SourceRange ConditionRange,
   ConditionValueKind ConditionValue) override {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

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

2020-07-13 Thread Zequan Wu via Phabricator via cfe-commits

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 them. If the comments are not important for those tests, maybe 
> they could be removed? As long as we keep one that tracks the intended 
> behavior of coverage on comments.


The comments in those coverage tests are used for FileCheck, like `//CHECK::`. 
So, we can't remove those ones.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-13 Thread Zequan Wu via Phabricator via cfe-commits

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());
 S.ActOnComment(Comment);

hans wrote:
> I don't think this is the right way to do it. It seems this callback is 
> intended for #if macros that exclude part of the file from preprocessing.
I believe we want to classify comments as `SkippedRegion`, 
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h#L217.
 
Added a new method `CommentSkipped` to add comment range to skipped ranges. 


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-13 Thread Zequan Wu via Phabricator via cfe-commits

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`, 
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h#L217
This will result overlapping `SkippedRegion` for comments and macro. Maybe, we 
shuold merge the overlapped `SkippedRegion` at the end.
Example:

  File 0, 1:12 -> 6:2 = #0
   Skipped,File 0, 2:3 -> 4:9 = 0
   Skipped,File 0, 2:15 -> 2:25 = 0
 1|  1|int main() {
 2|   |  #ifdef TEST // comment
 3|   |  int x = 1;
 4|   |  #endif
 5|  1|  return 0;
 6|  1|}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83592

Files:
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Parse/Parser.cpp


Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -34,6 +34,7 @@
   explicit ActionCommentHandler(Sema ) : S(S) { }
 
   bool HandleComment(Preprocessor , SourceRange Comment) override {
+PP.getPPCallbacks()->CommentSkipped(Comment);
 S.ActOnComment(Comment);
 return false;
   }
Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -38,6 +38,8 @@
   ArrayRef getSkippedRanges() const { return SkippedRanges; }
 
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
+
+  void CommentSkipped(SourceRange Range) override;
 };
 
 namespace CodeGen {
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -39,6 +39,10 @@
   SkippedRanges.push_back(Range);
 }
 
+void CoverageSourceInfo::CommentSkipped(SourceRange Range) {
+  SkippedRanges.push_back(Range);
+}
+
 namespace {
 
 /// A region of source code that can be mapped to a counter.
Index: clang/include/clang/Lex/PPCallbacks.h
===
--- clang/include/clang/Lex/PPCallbacks.h
+++ clang/include/clang/Lex/PPCallbacks.h
@@ -319,6 +319,10 @@
   virtual void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) {
   }
 
+  /// Hood called when the source range is comment, which should be skipped.
+  /// \param Range The SourceRange that is comment.
+  virtual void CommentSkipped(SourceRange Range) {}
+
   enum ConditionValueKind {
 CVK_NotEvaluated, CVK_False, CVK_True
   };
@@ -565,6 +569,11 @@
 Second->SourceRangeSkipped(Range, EndifLoc);
   }
 
+  void CommentSkipped(SourceRange Range) override {
+First->CommentSkipped(Range);
+Second->CommentSkipped(Range);
+  }
+
   /// Hook called whenever an \#if is seen.
   void If(SourceLocation Loc, SourceRange ConditionRange,
   ConditionValueKind ConditionValue) override {


Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -34,6 +34,7 @@
   explicit ActionCommentHandler(Sema ) : S(S) { }
 
   bool HandleComment(Preprocessor , SourceRange Comment) override {
+PP.getPPCallbacks()->CommentSkipped(Comment);
 S.ActOnComment(Comment);
 return false;
   }
Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -38,6 +38,8 @@
   ArrayRef getSkippedRanges() const { return SkippedRanges; }
 
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
+
+  void CommentSkipped(SourceRange Range) override;
 };
 
 namespace CodeGen {
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -39,6 +39,10 @@
   SkippedRanges.push_back(Range);
 }
 
+void CoverageSourceInfo::CommentSkipped(SourceRange Range) {
+  SkippedRanges.push_back(Range);
+}
+
 namespace {
 
 /// A region of source code that can be mapped to a counter.
Index: clang/include/clang/Lex/PPCallbacks.h
===
--- clang/include/clang/Lex/PPCallbacks.h
+++ clang/include/clang/Lex/PPCallbacks.h
@@ -319,6 +319,10 @@
   virtual void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) {
   }
 
+  /// Hood called when the source range is comment, which should be skipped.
+  /// \param Range The SourceRange that is comment.

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

2020-07-13 Thread Hans Wennborg via Phabricator via cfe-commits

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? As long as we keep one that tracks the intended behavior of 
coverage on comments.




Comment at: clang/lib/Parse/Parser.cpp:37
   bool HandleComment(Preprocessor , SourceRange Comment) override {
+PP.getPPCallbacks()->SourceRangeSkipped(Comment, Comment.getEnd());
 S.ActOnComment(Comment);

I don't think this is the right way to do it. It seems this callback is 
intended for #if macros that exclude part of the file from preprocessing.


Repository:
  rG LLVM Github Monorepo

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

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

2020-07-10 Thread Zequan Wu via Phabricator via cfe-commits

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

https://reviews.llvm.org/D83592

Files:
  clang/lib/Parse/Parser.cpp


Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -34,6 +34,7 @@
   explicit ActionCommentHandler(Sema ) : S(S) { }
 
   bool HandleComment(Preprocessor , SourceRange Comment) override {
+PP.getPPCallbacks()->SourceRangeSkipped(Comment, Comment.getEnd());
 S.ActOnComment(Comment);
 return false;
   }


Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -34,6 +34,7 @@
   explicit ActionCommentHandler(Sema ) : S(S) { }
 
   bool HandleComment(Preprocessor , SourceRange Comment) override {
+PP.getPPCallbacks()->SourceRangeSkipped(Comment, Comment.getEnd());
 S.ActOnComment(Comment);
 return false;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

30 matches

Site Navigation

Mail list logo

Footer information