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

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:471
+  std::string getConditionHeaderString(unsigned Condition) {
+    std::ostringstream ss;
+    ss << "Condition C" << Condition + 1 << " --> (";
----------------



================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:479
+  std::string getTestVectorHeaderString() {
+    std::ostringstream ss;
+    if (getNumTestVectors() == 0) {
----------------



================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:496
+           "TestVector index out of bounds!");
+    std::ostringstream ss;
+    // Add individual condition values to the string.
----------------



================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:533
+           "Condition index is out of bounds!");
+    std::ostringstream ss;
+
----------------



================
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
----------------
No need for `typedef` in C++.


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:262-271
+    _MCDCParameters()
+        : BitmapIdx(0), NumConditions(0), ID(0), TrueID(0), FalseID(0) {}
+    _MCDCParameters(unsigned BIdx, unsigned NC)
+        : BitmapIdx(BIdx), NumConditions(NC), ID(0), TrueID(0), FalseID(0) {}
+    _MCDCParameters(MCDC_Cond_ID ID, MCDC_Cond_ID TID, MCDC_Cond_ID FID)
+        : BitmapIdx(0), NumConditions(0), ID(ID), TrueID(TID), FalseID(FID) {}
+    _MCDCParameters(unsigned BIdx, unsigned NC, MCDC_Cond_ID ID,
----------------
I'd consider omitting these constructors altogether and just use aggregate initialization with default values.


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:394
+struct MCDCRecord {
+  typedef enum { MCDC_DontCare = -1, MCDC_False = 0, MCDC_True = 1 } CondState;
+
----------------
No need for `typedef` in C++.


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:435
+
+  bool isCondIndepPairCovered(unsigned Condition) const {
+    // Accessing conditions in the TestVector Row Pairs requires a translation
----------------
I'd consider spelling it out in full, it took me a second to realize that `Indep` means "Independent", or at least I assume it does?


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:206
+  BitVector Result(SizeInBits, false);
+  for (auto byte = std::rbegin(Bytes); byte != std::rend(Bytes); ++byte) {
+    uint32_t data = *byte;
----------------



================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:207
+  for (auto byte = std::rbegin(Bytes); byte != std::rend(Bytes); ++byte) {
+    uint32_t data = *byte;
+    Result <<= CHAR_BIT;
----------------



================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:363
+    // For each condition.
+    for (unsigned c = 0; c < NumConditions; c++) {
+      bool pair_found = false;
----------------



================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:364
+    for (unsigned c = 0; c < NumConditions; c++) {
+      bool pair_found = false;
+
----------------



================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:367
+      // For each executed test vector.
+      for (unsigned i = 0; !pair_found && i < NumTVs; i++) {
+
----------------



================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:370
+        // Compared to every other executed test vector.
+        for (unsigned j = 0; !pair_found && j < NumTVs; j++) {
+          if (i == j)
----------------



================
Comment at: llvm/tools/llvm-cov/CoverageSummaryInfo.cpp:50
+  for (const auto &Record : Records)
+    for (unsigned c = 0; c < Record.getNumConditions(); c++) {
+      if (!Record.isCondFolded(c))
----------------



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

https://reviews.llvm.org/D138847