vsk added a comment.
Thanks for the patient explanation.
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1391
+ // Clear DeferredRegion because we don't need to complete it after switch.
+ DeferredRegion = None;
+
----------------
Is the reason why we don't need to complete it because the logic in VisitStmt
handles filling in the gap region? What happens if this isn't cleared, or if
there are nested switches?
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:942
pushRegion(Counter::getZero());
- auto &ZeroRegion = getRegion();
- ZeroRegion.setDeferred(true);
- LastTerminatedRegion = {EndLoc, RegionStack.size()};
+ if (!HasTerminateStmt) {
+ auto &ZeroRegion = getRegion();
----------------
zequanwu wrote:
> vsk wrote:
> > zequanwu wrote:
> > > vsk wrote:
> > > > What's supposed to be the difference between the zero region pushed
> > > > after a `return;` vs. after a `break;`?
> > > What I think is `DeferredRegion` is only used for `break;` and
> > > `continue`. Other terminate statements like `return;`, `throw` etc will
> > > use the logic in `VisitStmt` to emit gap region. So, I added this
> > > condition to separate the two cases.
> > What do you think of the notion of using the gaps inserted in VisitStmt to
> > replace the whole deferred region system? Is it something that might be
> > feasible (if perhaps out of scope for this patch), or do you see a
> > fundamental reason it can't/shouldn't be done?
> I think it is feasible. For break and continue, they only affect the
> statements after them in the same block. For other terminate statements, they
> affect all the statements after them even outside the block, see example
> below. Also instead of emitting a zero gap region in VisitStmt, we need emit
> gap region with (previous counter - current statement counter).
>
> ```
> while (cond) {
> ...
> break; // This affects statements' count until the end of the while body.
> ...
> }
>
> while (cond) {
> ...
> return; // This affects statements' count until the end of the function body.
> ...
> }
> ```
>
Thanks, that's really helpful. It's clicking now that the main difference is
that the deferred region sticks around after we're done visiting a block.
================
Comment at: clang/test/CoverageMapping/classtemplate.cpp:83
std::map<int, int> Test4() { // CHECK-INIT-LIST: File 0, [[@LINE]]:28 ->
[[@LINE+3]]:2 = #0
- abort();
+ abort(); // CHECK-INIT-LIST-NEXT: Gap,File 0,
[[@LINE]]:11 -> [[@LINE+1]]:3 = 0
return std::map<int, int>{{0, 0}}; // CHECK-INIT-LIST-NEXT: [[@LINE]]:3 ->
[[@LINE]]:36 = 0
----------------
Nice, this is from the special handling of noreturn calls.
================
Comment at: clang/test/CoverageMapping/deferred-region.cpp:19
return;
- } // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+2]]:3 = (#0 - #1)
+ } // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+2]]:3 = 0
----------------
Nice, this should amount to the same end result, it's a smaller encoding.
================
Comment at: clang/test/CoverageMapping/deferred-region.cpp:43
if (true)
- return; // CHECK: Gap,File 0, [[@LINE]]:11
+ return;
else
----------------
I'm confused by this change. Do we lose the gap here? I assumed it was needed
to prevent the condition count from `if (true)` from kicking in after the
`return`?
================
Comment at: clang/test/CoverageMapping/switch.cpp:62
default: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = #4
break; // CHECK-NEXT: Branch,File 0, [[@LINE-1]]:3 ->
[[@LINE-1]]:10 = #4, (#0 - #4)
+ }
----------------
The blank space after `default: break;` and before the closing '}' should have
the same count as the switch condition, I thought (this goes for all of the
unreachable code within a switch body)? The idea is to prevent 'not-executed'
regions from appearing after the `break` stmt.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97101/new/
https://reviews.llvm.org/D97101
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
