[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/include/llvm/ProfileData/Coverage/CoverageMapping.h:255
+    /// Byte Index of Bitmap Coverage Object for a Decision Region (MC/DC
+    /// only).
+    unsigned BitmapIdx = 0;
----------------
Would it make sense to drop "(MC/DC only)" on this and other member vars in this class?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:387
+struct MCDCRecord {
+  enum CondState { MCDC_DontCare = -1, MCDC_False = 0, MCDC_True = 1 };
+
----------------
Can you put a doxygen comment on this explaining what each of the members are?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:418
+  CondState getTVCondition(unsigned TestVectorIndex, unsigned Condition) {
+    // Accessing conditions in the TestVectors requires a translation from a
+    // ordinal position to actual condition ID. This is done via PosToID[].
----------------
I think this comment should be changed into a Doxygen comment that explains what this function is doing to TestVectorIndex and Condition.


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:423
+
+  CondState getTVResult(unsigned TestVectorIndex) {
+    // The last value for a Test Vector, after its constituent conditions, is
----------------
Could use a doxygen comment here


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:429
+
+  bool isConditionIndependencePairCovered(unsigned Condition) const {
+    // Accessing conditions in the TestVector Row Pairs requires a translation
----------------
doxygen


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:439
+
+  TVRowPair getConditionIndependencePair(unsigned Condition) {
+    // Accessing conditions in the TestVector Row Pairs requires a translation
----------------
doxygen


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:450
+    unsigned Covered = 0;
+    for (unsigned C = 0; C < getNumConditions(); C++) {
+      if (isCondFolded(C))
----------------
Would it be valid for `getNumConditions` to ever be 0? Would it make sense to add an assert somewhere if it doesn't make sense?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:508
+      }
+      if (Condition != getNumConditions() - 1)
+        OS << ",  ";
----------------
might as well pull this into a variable instead of calling it here and in the loop

```
const auto NumConditions = getNumConditions();
```


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:331
+
+  void buildTestVector(MCDCRecord::TestVector &TV, unsigned ID = 1) {
+    shouldCopyOffTestVectorForTruePath(TV, ID);
----------------
simple explanation in doxygen comment?


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:340
+  void findExecutedTestVectors(BitVector &ExecutedTestVectorBitmap) {
+    // Walk the bits in the bitmap.  A bit set to '1' indicates that the test
+    // vector at the corresponding index was executed during a test run.
----------------
this should be a doxygen comment imo


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:350
+
+  // For a given condition and two executed Test Vectors, A and B, see if the
+  // two test vectors match forming an Independence Pair for the condition.
----------------
doxygenify


================
Comment at: llvm/tools/llvm-cov/CodeCoverage.cpp:382
+
+    if (!ViewMCDCRecords.empty()) {
+      auto SubView = SourceCoverageView::create(SourceName, File, ViewOpts,
----------------
can reduce indentation here

```
if (ViewMCDCRecords.empty())
  return;
auto ...
```


================
Comment at: llvm/tools/llvm-cov/CoverageSummaryInfo.h:145
 
+/// Provides information about branches coverage for a function/file.
+class MCDCCoverageInfo {
----------------
might as well say this is MC/DC here


================
Comment at: llvm/tools/llvm-cov/CoverageSummaryInfo.h:148
+  /// The number of Independence Pairs that were covered.
+  size_t CoveredPairs;
+
----------------
init to 0?


================
Comment at: llvm/tools/llvm-cov/CoverageSummaryInfo.h:151
+  /// The total number of Independence Pairs in a function/file.
+  size_t NumPairs;
+
----------------
init to 0?


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

https://reviews.llvm.org/D138847