alanphipps marked 2 inline comments as done.
alanphipps added inline comments.
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1169
+ createBranchRegion(S->getCond(), BodyCount,
+ subtractCounters(CondCount, BodyCount));
}
----------------
vsk wrote:
> If `theWhileStmt->getCond()` is-a BinaryOperator, it would not be considered
> an instrumented condition and no branch region would be created here. OTOH,
> if the condition is instrumented (e.g. as it would be in `while (x);`), a
> branch region would be created.
>
> Is this the expected outcome? It seems a little bit inconsistent compared to
> either a) allowing the RecursiveASTVisitor to identify expressions that
> require branch regions, or b) guaranteeing that each while statement comes
> with a branch region for its condition.
>
> I have the same question about the `createBranchRegion` calls in VisitDoStmt,
> VisitForStmt, etc. (But I'm not concerned about the calls to
> `createBranchRegion` in VisitBinL{And,Or} and VisitSwitch*.)
Right, that's the expected outcome, but I think I see what you're saying in
that it could be confusing to call "createBranchRegion()" that may not actually
create a branch region in some cases.
I could move the check for isInstrumentedCondition() out of
createBranchRegion() to make it clear that a branch region is only created for
when that is TRUE, but I wanted to encapsulate as much as I could in the
function to avoid duplication. Perhaps it would be better to rename
"createBranchRegion()" to something more like
"createBranchRegionForInstrumentedCond()"?
The same behavior exists for VisitBinL{And,Or} since those conditions could
also be nested BinaryOperators.
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1277
+ // Create Branch Region around condition.
+ createBranchRegion(S->getCond(), BodyCount,
+ subtractCounters(LoopCount, BodyCount));
----------------
vsk wrote:
> Is the condition of a CXXForRangeStmt something that's visible to the user?
> It looks more like a clang implementation detail (but I could be mistaken).
Technically the condition isn't visible, per se, but the range-based construct
implies a branchable condition that other vendors do track, so I still think
it's useful to show it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84467/new/
https://reviews.llvm.org/D84467
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
