[PATCH] D138847: MC/DC in LLVM Source-Based Code Coverage: llvm-cov visualization

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:336
+public:
+  MCDCRecord processMCDCRecord() {
+    unsigned i = 0;
----------------
Doxygen comment?


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:341
+
+    // Walk the Record's BranchRegions (representing Conditions) in order to:
+    // - Hash the condition based on its corresponding ID. This will be used to
----------------
this comment probably could be adapted into a doxygen comment


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:481
+      return Error::success();
+    } else if (IPE != instrprof_error::unknown_function)
+      return make_error<InstrProfError>(IPE);
----------------
LLVM coding standards say to avoid `else` after `return`.

https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:529
+      MCDCBranches.push_back(Region);
+      if (--NumConds == 0) {
+        // Evaluating the test vector bitmap for the decision region entails
----------------
Can you explain why you are decrementing this?


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:536
+        Expected<BitVector> Bitmap =
+            Ctx.evaluateBitmap(MCDCDecision->MCDCParams.BitmapIdx,
+                               MCDCDecision->MCDCParams.NumConditions);
----------------
Why not rewrite `evaluateBitmap` so that it passes the whole `MCDCDecision` struct?




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

https://reviews.llvm.org/D138847