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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1169
+    createBranchRegion(S->getCond(), BodyCount,
+                       subtractCounters(CondCount, BodyCount));
   }
----------------
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*.)


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1277
+    // Create Branch Region around condition.
+    createBranchRegion(S->getCond(), BodyCount,
+                       subtractCounters(LoopCount, BodyCount));
----------------
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).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84467/new/

https://reviews.llvm.org/D84467