[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Alan Phipps via Phabricator via llvm-commits]

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