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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In D83592#2148638 <https://reviews.llvm.org/D83592#2148638>, @zequanwu wrote:

> 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`.


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<CounterMappingRegion>
+mergeSkippedRegions(MutableArrayRef<CounterMappingRegion> MappingRegions) {
+  SmallVector<CounterMappingRegion, 32> 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