[PATCH] D125779: [llvm-debuginfo-analyzer] 04 - Locations and ranges

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVRange.h:48
+// Class to represent a list of range addresses associated with a
+// scope; the addresses are stored in ascending order.
+using LVRangeEntries = std::vector<LVRangeEntry>;
----------------
Can they overlap?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:122
+  void printActiveRanges(raw_ostream &OS, bool Full = true) const {
+    (const_cast<LVScope *>(this))->printActiveRanges(OS, Full);
+  }
----------------
I think I said something about this in a previous patch, that a non-const print function is counter-intuitive.  If it can be designed not to need a non-const version, that would be better.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:224
+  // Calculate coverage factor.
+  void CalculateCoverageFactor() {
+    float CoveragePercentage = 0;
----------------
Why have both `calculateCoverage` and `CalculateCoverageFactor` ?  They appear to do the same thing.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:183
+// Return the original CodeView enum value.
+inline uint16_t updateOperationCode(uint8_t Code) { return 0x1100 | Code; }
+
----------------
`updateOperationCode` implies modifying something, which this function does not do.  `getCVOperationCode` maybe?


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:623
+void LVLocation::print(LVLocations *Locations, raw_ostream &OS, bool Full) {
+  assert(Locations && "Locations must not be nullptr");
+  // Print the symbol coverage.
----------------
This is okay, but when a pointer parameter must not be null, it's better practice to make it a reference parameter instead.  Then tests for non-null go away.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:626
+  if (Locations && !Locations->empty() && options().getAttributeCoverage()) {
+    assert(!Locations->empty() && "Locations must have at least one element");
+    // The location entries are contained within a symbol. Get a location,
----------------
The `if` above already checked `!Locations->empty()` so this assertion is redundant.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVRange.cpp:26
+
+  LLVM_DEBUG({ dbgs() << "\nRanges Tree:\n"; });
+
----------------
The same heading is here and below where the entire resulting tree is printed. Maybe these should be different?  "Range Tree entries" here, and leave "Ranges Tree" for when the whole tree is printed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVRange.cpp:118
+
+// Find the associated Scope for the given ranges values.
+bool LVRange::findEntry(LVAddress LowerAddress, LVAddress UpperAddress) const {
----------------
This comment does not describe what the method does, from the caller's perspective.  Also maybe should be named `hasEntry` instead of `findEntry` 


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVRange.cpp:131
+                              const LVRangeEntry &rhs) -> bool {
+    return (lhs.lower() < rhs.lower());
+  };
----------------
I see the caller is `stable_sort` so is there an ordering guarantee for multiple ranges with the same lower bound?  If so, where is that documented and enforced?


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:712
+// Get all the locations associated with symbols.
+void LVScope::getLocations(LVLocations &LocationList,
+                           LVValidLocation ValidLocation, bool RecordInvalid) {
----------------
"get" usually implies a method that is returning data of interest, but these are all doing something else.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:762
+LVScope *LVScope::outermostParent(LVAddress Address) {
+  LVScope *Parent = this;
+  while (Parent) {
----------------
Looks like if Ranges is null, this ends up returning null.  So, cheaper to do
```
if (!Ranges)
  return nullptr;
```
and then remove the `if (Ranges)` from inside the while loop.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:942
+    bool RecordInvalid = false;
+    getRanges(Locations, ValidLocation, RecordInvalid);
+  }
----------------
It looks odd to call something named `getRanges` using local variables that are immediately thrown away.  This is essentially the same comment about the names of the `get` methods I made previously.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp:190
+      // lower address.
+      // The symbol can have a set of non-continuos locations. We are using
+      // only the first location entry to get the outermost parent.
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp:203
+        CoverageParent
+            ? rint((double(CoverageFactor) / CoverageParent) * 100.0 * 100.0) /
+                  100.0
----------------
Same comment as the other place that made this same calculation, in a previous patch.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/LocationRangesTest.cpp:58
+    T *Element = new (std::nothrow) T();
+    EXPECT_NE(Element, nullptr);
+    return Element;
----------------
Use `ASSERT_NE` so the test will abort if the allocation fails.


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

https://reviews.llvm.org/D125779