[PATCH] D125778: [llvm-debuginfo-analyzer] 03 - Logical elements

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h:84
+  void print(raw_ostream &OS, bool Full = true) const override;
+  void printExtra(raw_ostream &OS, bool Full = true) const override{};
+
----------------



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:155
+  // The parent of this object (nullptr if the root scope). For locations,
+  // the parent it is a symbol object; otherwise is a scope object.
+  union {
----------------



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:108
+  virtual void printSizes(raw_ostream &OS) const {};
+  virtual void printSummary(raw_ostream &OS) const {};
+
----------------
No `;` after the `{}` 


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:236
+  void printExtra(raw_ostream &OS, bool Full = true) const override;
+  virtual void printMatchedElements(raw_ostream &OS, bool UseMatchedElements){};
+
----------------



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:518
+
+// Class to represent a DWARF subroutine function.
+class LVScopeFunctionType final : public LVScopeFunction {
----------------
Did you mean "subroutine type"?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:552
+
+class LVScopeRoot final : public LVScope {
+  size_t FileFormatNameIndex = 0;
----------------
Needs comment saying what does LVScopeRoot represent.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:39
+    for (auto *Item : *this)
+      delete Item;
+  }
----------------
This appears to be assuming that the vector elements are pointers?  And the memory for the pointed-to objects is owned by the vector?  I'd think it would be simpler to have a SmallVector of objects, not pointers-to-objects, in which case SmallVector takes care of destroying them for you.
(Or, use a SmallVector of smart pointers.)

If you really want a SmallVector of normal pointers, then the template needs to guarantee that T is a pointer type, somehow.  But this all feels rather fragile.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:51
+  void set(T Idx) {
+    assert(static_cast<unsigned>(Idx) < static_cast<unsigned>(T::LastEntry) &&
+           "Invalid property");
----------------
I think the `static_cast<unsigned>` should be unnecessary? because Idx has type T, therefore `Idx < T::LastEntry` should work correctly.
Same comment in the other methods.

Actually, you could defer the bounds checking to SmallBitVector's `operator[]` and remove a lot of this code.  For example
```
void set(T Idx) {
  Bits[static_cast<unsigned>(Idx)] = 1;
}
```


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:64
+    assert(static_cast<unsigned>(Idx) < static_cast<unsigned>(T::LastEntry) &&
+           "Invalid property");
+    return Bits[static_cast<unsigned>(Idx)];
----------------
This assert is redundant, `SmallBitVector::operator[]` does the same assertion.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h:88
+    assert((!Element || (Element && static_cast<LVSymbol *>(Element))) &&
+           "Invalid element");
+    setReference(static_cast<LVSymbol *>(Element));
----------------
This assertion looks a little strange.  I think it's trying to verify that Element is either null, or is-a LVSymbol.  In that case the assert would be
`assert((!Element || dyn_cast<LVSymbol *>(Element)) && "Invalid element");`


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:28
+             : nullptr;
+}
+
----------------
If the LVElement hierarchy is set up for LLVM-style casting, then this could be just `return dyn_cast<LVType *>(ElementType);`


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:33
+             ? static_cast<LVScope *>(ElementType)
+             : nullptr;
+}
----------------
If the LVElement hierarchy is set up for LLVM-style casting, then this could be just `return dyn_cast<LVScope *>(ElementType);`


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:97
+void LVElement::setFilename(StringRef Filename) {
+  // Get index for the flatted out filename.
+  FilenameIndex = getStringIndex(Filename);
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp:40
+const char *LVLine::kind() const {
+  static const char *Kind = KindUndefined;
+  if (getIsLineDebug())
----------------
`Kind` doesn't need to be `static` here.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:56
+const char *LVScope::kind() const {
+  static const char *Kind = KindUndefined;
+  if (getIsArray())
----------------
`Kind` doesn't need to be `static` here.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:411
+StringRef LVScope::resolveReferencesChain() {
+  // If the scope have a DW_AT_specification or DW_AT_abstract_origin,
+  // follow the chain to resolve the name from those references.
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:638
+  while (Parent) {
+    // Terminates if the 'SetFunction' have been already executed.
+    if ((Parent->*GetFunction)())
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:652
+    while (Parent) {
+      // Terminates if the 'SetFunction' have been already executed.
+      if ((Parent->*GetFunction)())
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:922
+
+void LVScopeCompileUnit::printScopeSize(LVScope *Scope, raw_ostream &OS) {
+  LVSizesMap::iterator Iter = Sizes.find(Scope);
----------------
Could this be `const LVScope *Scope` ? Which would avoid some casting in printSizes.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:930
+    float Percentage =
+        rint((float(Size) / CUContributionSize) * 100.0 * 100.0) / 100.0;
+    OS << format("%10d (%6.2f%%) : ", Size, Percentage);
----------------
Did this solve a real problem?  I would expect this expression to be folded to
`rint((float(Size) / CUContributionSize) * 100.0)`
but maybe my notion of floating-point constant folding is too naive.



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:947
+  // Recursively print the contributions for each scope.
+  std::function<void(LVScope * Scope)> PrintScope = [&](LVScope *Scope) {
+    if (Scope->getLevel() < options().getOutputLevel()) {
----------------
Is there a reason this isn't a lambda?

Also, if the parameter is const then some of the casting below can go away.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:969
+  printScopeSize(const_cast<LVScopeCompileUnit *>(this), OS);
+  PrintScope(const_cast<LVScopeCompileUnit *>(this));
+
----------------
Comments above would allow removing these const_cast calls.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1086
+  const_cast<LVScopeCompileUnit *>(this)->Found.reset();
+  const_cast<LVScopeCompileUnit *>(this)->Printed.reset();
+
----------------
Looks like the method shouldn't be declared const.  Ordinarily you wouldn't think a print() method would be non-const, but obviously this one is.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1145
+        if (Scope->getHasReferenceAbstract() && !Scope->getAddedMissing())
+          // Add missing elements at the function nested scopes.
+          Scope->addMissingElements(Scope->getReference());
----------------
This looks like it just repeats the comment from a few lines above.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp:23
+
+// Transforms '\\' into '/' (Platform independent).
+std::string llvm::logicalview::transformPath(StringRef Path) {
----------------
There should be existing functions in Support for this.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp:54
+  std::replace(Name.begin(), Name.end(), '"', '_');
+  std::replace(Name.begin(), Name.end(), ' ', '_');
+  return Name;
----------------
While this is easy to understand, it's iterating the string 13 times. Maybe a lambda with a switch statement, and one call to `transform` ?  Or a lambda that calls strpbrk.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp:35
+const char *LVSymbol::kind() const {
+  static const char *Kind = KindUndefined;
+  if (getIsCallSiteParameter())
----------------
`Kind` doesn't need to be `static` here.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:48
+const char *LVType::kind() const {
+  static const char *Kind = KindUndefined;
+  if (getIsBase())
----------------
`Kind` doesn't need to be `static` here.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:90
+  // any reference tags, such as DW_AT_specification, DW_AT_abstract_origin,
+  // DW_AT_extension.
+
----------------
This comment might belong in another patch? It doesn't seem to describe anything in this method.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:134
+  // In the case of unnamed types, try to generate a name for it, using
+  // the parents name and the line information. Ignore the template
+  if (!isNamed() && !getIsTemplateParam())
----------------
Ignore the template parameters. (?)


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:273
+      } else
+        Name.append(std::string(ArgType->getName()));
+    } else {
----------------
Even though it's a one-line else clause, please put `{}` around it so that the next line doesn't look like incorrect indentation.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp:65
+    T *Element = new (std::nothrow) T();
+    EXPECT_NE(Element, nullptr);
+    return Element;
----------------
I think `ASSERT_NE` here would be better; bail out of the test in a controlled way if the `new` returns nullptr.  Otherwise you'll just get a segfault somewhere later that will be harder to diagnose.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp:111
+  Root = getScopesRoot();
+  EXPECT_NE(Root, nullptr);
+
----------------
I think `ASSERT_NE` here too.


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

https://reviews.llvm.org/D125778