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

Zequan Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 13 15:37:30 PDT 2020


zequanwu marked an inline comment as done.
zequanwu added a comment.

In D83592#2148376 <https://reviews.llvm.org/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<CounterMappingRegion>
+mergeSkippedRegions(MutableArrayRef<CounterMappingRegion> MappingRegions) {
+  SmallVector<CounterMappingRegion, 32> 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





More information about the cfe-commits mailing list