[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:250
+  using MCDC_Cond_ID = unsigned int;
+  typedef struct _MCDCParameters {
+    /// Byte Index of Bitmap Coverage Object for a Decision Region (MC/DC
----------------
Style nit: I don't think I've ever seen an underscore at the beginning of a struct name? Maybe I'm wrong.


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:396
+
+  using TestVector = std::vector<CondState>;
+  using TestVectors = std::vector<TestVector>;
----------------
Why not `SmallVector`?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:397
+  using TestVector = std::vector<CondState>;
+  using TestVectors = std::vector<TestVector>;
+  using BoolVector = std::vector<bool>;
----------------
Why not `SmallVector`?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:398
+  using TestVectors = std::vector<TestVector>;
+  using BoolVector = std::vector<bool>;
+  using TVRowPair = std::pair<unsigned, unsigned>;
----------------
Why not `SmallVector`?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:454
+  float getPercentCovered() const {
+    unsigned folded = 0;
+    unsigned covered = 0;
----------------
Variables in this function should start with a capital letter.

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

> Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats).





================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:466
+      return 0.0;
+    return ((double)(covered) / (double)(total)) * 100.0;
+  }
----------------
Nit: prefer static cast for greppability


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:469
+
+  std::string getConditionHdrStr(unsigned Condition) {
+    std::ostringstream ss;
----------------
Nit: Prefer whole words? (Ditto for the other `Hdr` function)


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:492
+
+  std::string getTestVectorStr(unsigned TestVector) {
+    assert(TestVector < getNumTestVectors());
----------------
Nit: prefer whole words?

Also when I read "TestVector" I assume the type is going to be some sort of vector. Could you give this a different name?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:493
+  std::string getTestVectorStr(unsigned TestVector) {
+    assert(TestVector < getNumTestVectors());
+    std::ostringstream ss;
----------------
In asserts, a string describing why the assert is there can be very helpful.


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:495
+    std::ostringstream ss;
+    // Print Individual Conditions
+    ss << "  " << TestVector + 1 << " { ";
----------------
Maybe there should be a function called `printConditions`?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:517
+
+    // Print Result
+    ss << "  = ";
----------------
Maybe there should be a function called `printResult`?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:528
+
+  std::string getCondCoverageStr(unsigned Condition) {
+    assert(Condition < getNumConditions());
----------------



================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:529
+  std::string getCondCoverageStr(unsigned Condition) {
+    assert(Condition < getNumConditions());
+    std::ostringstream ss;
----------------



================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:212
+
+class MCDCRecordProcessor {
+  BitVector &Bitmap;
----------------
Can you add Doxygen comments to the member variables in this class?


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:213
+class MCDCRecordProcessor {
+  BitVector &Bitmap;
+  CounterMappingRegion &Region;
----------------
Looking at the later code, it looks like this indicates whether or not something was run. How about renaming it so that's clear?

E.g. `VectorAtIdxWasExecuted` or something?


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:238
+    // MCDC Region's bitmap (see findExecutedTestVectors()).
+    unsigned idx = 0;
+    for (auto Cond = std::rbegin(TV); Cond != std::rend(TV); ++Cond) {
----------------
LLVM style guide for variables


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:256
+
+    // An ID of '0' indicates the end of a test vector, at which point, the
+    // test vector can be copied off, and the algorithm can keep going.
----------------
How about some helper functions for this instead of a comment?

```
shouldCopyOffTestVectorForTruePath(Branch);
shouldCopyOffTestVectorForFalsePath(Branch);
```


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:281
+    for (unsigned i = 0; i < Bitmap.size(); i++)
+      if (Bitmap[i] == 1) {
+        assert(!TestVectors[i].empty() && "Test Vector doesn't exist.");
----------------
How about let's get rid of the braces here to make this a little easier to read.

```
for (unsigned Idx = 0; Idx < Bitmap.size(); ++Idx) {
   if (!Bitmap[Idx])
     continue;
   assert(!TestVectors[i].empty() && "Test vector doesn't exist.");
   ExecVectors.push_back(TestVectors[i]);
}
```


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:287
+
+  bool matchTestVectors(unsigned Aidx, unsigned Bidx, unsigned C) {
+    const MCDCRecord::TestVector &A = ExecVectors[Aidx];
----------------
Can you add a comment explaining what this function is supposed to do?

Also can you give a more descriptive name to `C`? Maybe `ConditionIdx`?


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:303
+        continue;
+      if (A[i] != MCDCRecord::MCDC_DontCare &&
+          B[i] != MCDCRecord::MCDC_DontCare && A[i] != B[i])
----------------
How about

```
// Look for other conditions that don't match.
// Skip over C and don't care values (because we don't care about them).
const auto ARecordTyForCond = A[i];
const auto BRecordTyForCond = B[i];
if (i == C ||
    ARecordTyForCond == MCDCRecord::MCDC_DontCare ||
    BRecordTyForCond == MCDCRecord::MCDC_DontCare)
  continue;

// Condition mismatch, so we don't have a match.
if (ARecordTyForCond != BRecordTyForCond)
  return false;
```

The checks for don't-cares are technically "skip this" statements, so it feels clearer to include them in the `continue` branch.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:312
+
+  void findIndependencePairs() {
+    unsigned NumTVs = ExecVectors.size();
----------------
Can you add a comment to this function?


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

https://reviews.llvm.org/D138847