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

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