[PATCH] D125782: [llvm-debuginfo-analyzer] 07 - Compare elements

Tue Sep 20 10:01:18 PDT 2022

probinson added inline comments.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h:35
+  // the comparison pass.
+  // By recording the missing/added elements along with its pass, it
+  // allow to check which elements were missing/added during each pass.
----------------

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h:36
+  // By recording the missing/added elements along with its pass, it
+  // allow to check which elements were missing/added during each pass.
+  LVPassTable PassTable;
----------------

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h:62
+
+public:
+  static LVCompare &getInstance();
----------------
No need to repeat `public:`

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h:75
+  }
+  LVPassTable getPassTable() const { return PassTable; }
+
----------------
Returning a std::vector by value seems inefficient.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVElement.h:345
+  // Report the element as missing or added during comparison.
+  virtual void report(LVComparePass Pass){};
+
----------------

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:300
+
+  // Returns true if 'current' scope is equal to the given 'scope'.
+  virtual bool equalNumberOfChildren(const LVScope *Scope) const;
----------------
Does this comment apply to all the methods in this block of code?  It seems like not, in which case it would be good to put a blank line before the declarations that it does not apply to.  (Or even better, comments for each of the other methods!)

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:307
+
 public:
   static LVScopeDispatch &getDispatch() { return Dispatch; }
----------------
No need to repeat `public:`

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:351
+  bool equals(const LVScope *Scope) const override;
+  LVScope *equals(const LVScopes *Scopes) const override;
+
----------------
The comment does not describe the second overload; put in a blank line, or preferably a comment for the second one.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:652
+  bool equals(const LVScope *Scope) const override;
+  LVScope *equals(const LVScopes *Scopes) const override;
+
----------------
Comment does not apply to the second overload.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:692
+  bool equals(const LVScope *Scope) const override;
+  LVScope *equals(const LVScopes *Scopes) const override;
+
----------------
Comment does not apply to the second overload.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:730
+  bool equals(const LVScope *Scope) const override;
+  LVScope *equals(const LVScopes *Scopes) const override;
+
----------------
Comment does not apply to the second overload.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h:180
+  bool equals(const LVSymbol *Symbol) const;
+  static bool equals(const LVSymbols *References, const LVSymbols *Targets);
+  void report(LVComparePass Pass) override;
----------------
Comment does not apply to the second overload.
Also curious that this time the second overload is `static` when for other classes it is not.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h:183
+
 public:
   void print(raw_ostream &OS, bool Full = true) const override;
----------------
no need to keep repeating `public:`

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVType.h:134
+  virtual bool equals(const LVType *Type) const;
+  static bool equals(const LVTypes *References, const LVTypes *Targets);
+  void report(LVComparePass Pass) override;
----------------
Comment does not apply to the second overload.
Which is also `static` ?

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVCompare.cpp:35
+
+constexpr unsigned getHeader() {
+  return static_cast<unsigned>(LVCompareIndex::Header);
----------------
These appear to be global functions; `constexpr` implies `inline` but not `static`.
They should be `static` or inside the anonymous namespace.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVCompare.cpp:48
+
+LVCompare *CurrentComparator = nullptr;
+LVCompare &LVCompare::getInstance() {
----------------
`CurrentComparator` is a global variable, not even inside any namespace.  Could it be a static member inside LVCompare?

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp:78
+LVLine *LVLine::findIn(const LVLines *Targets) const {
+  if (Targets) {
+    LLVM_DEBUG({
----------------
```
if (!Targets)
  return nullptr;
```
allows the rest of the function to be less indented.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp:103
+
+bool LVLine::equals(const LVLines *References, const LVLines *Targets) {
+  if (!References && !Targets)
----------------
Could this method be marked `const` ?

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:826
+LVScope *LVScope::findIn(const LVScopes *Targets) const {
+  if (Targets) {
+    // In the case of overloaded functions, sometimes the DWARF used to
----------------
Can reduce indentation by starting with
```
if (!Targets)
  return nullptr;
```

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:861
+
+// Returns true if 'current' scope is equal to the given 'scope'.
+bool LVScope::equalNumberOfChildren(const LVScope *Scope) const {
----------------

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:900
+                                 bool TraverseChildren) {
+  LLVM_DEBUG({
+    if (References && Targets) {
----------------
Could simplify slightly by starting with
```
if (!(References && Targets))
  return;
```

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:917
+  });
+  if (References && Targets)
+    for (LVScope *Reference : *References) {
----------------
If you don't simplify as suggested above, this `if` needs braces around the `for` per LLVM style.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:966
+
+bool LVScope::equals(const LVScopes *References, const LVScopes *Targets) {
+  if (!References && !Targets)
----------------
Could this method be marked `const` ?

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1048
+    // Compare full name.
+    if (getFilenameIndex() != Scope->getFilenameIndex())
+      return false;
----------------
This sequence looks a bit strange; if neither one is named, compare the filenames? If that's correct maybe the comment should say "file name" instead of "full name"?

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1054
+
+LVScope *LVScopeAggregate::equals(const LVScopes *Scopes) const {
+  assert(Scopes && "Scopes must not be nullptr");
----------------
The name `equals` implies a straightforward equality test, which is not really what this method does.  In fact it's a little hard for me to describe what it does...  looks through a given set of Scopes to find one that equals the current Scope?  So maybe `findEqualScope` would be a better name?

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1309
 // Increase the added element counters, for printing summary.
-void LVScopeCompileUnit::addedElement(LVLine *Line) { increment(Line); }
-void LVScopeCompileUnit::addedElement(LVScope *Scope) { increment(Scope); }
-void LVScopeCompileUnit::addedElement(LVSymbol *Symbol) { increment(Symbol); }
-void LVScopeCompileUnit::addedElement(LVType *Type) { increment(Type); }
+// Notify the Reader if element comparison.
+void LVScopeCompileUnit::addedElement(LVLine *Line) {
----------------
Not sure what "Notify the Reader if element comparison" means.  It looks like what the code does is "Notify the Reader of the new element."

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1807
+
+LVScope *LVScopeFunction::equals(const LVScopes *Scopes) const {
+  assert(Scopes && "Scopes must not be nullptr");
----------------
Same comment as `LVScopeAggregate::equals`

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1876
+
+LVScope *LVScopeFunctionInlined::equals(const LVScopes *Scopes) const {
+  return LVScopeFunction::equals(Scopes);
----------------
Same comment as `LVScopeAggregate::equals`

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1939
+
+LVScope *LVScopeNamespace::equals(const LVScopes *Scopes) const {
+  assert(Scopes && "Scopes must not be nullptr");
----------------
Same comment as `LVScopeAggregate::equals`

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1054
+LVScope *LVScopeAggregate::equals(const LVScopes *Scopes) const {
+  for (LVScope *Scope : *Scopes)
+    if (equals(Scope))
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > I believe this is a good idea to always use `assert` on a pointer if there is a dereference of or call a method of in the function to be sure the pointer is not null. The `LLVM Coding Standards` guideline recommends to use asserts liberally: "Check all of your preconditions and assumptions, you never know when a bug (not necessarily even yours) might be caught early by an assertion, which reduces debugging time dramatically." 
> Very good point.
> Added `assert(Scopes && "Scopes must not be nullptr");` in few other places as well.
> 
When pointers cannot be null it's usually better practice to pass a reference instead.  I understand that this would be a fairly extensive API change, so I won't insist.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp:285
+                                  const LVSymbols *Targets) {
+  LLVM_DEBUG({
+    if (References && Targets) {
----------------
Could simplify/reduce indentation by starting with
```
if (!(References && Targets))
  return;
```

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp:311
+LVSymbol *LVSymbol::findIn(const LVSymbols *Targets) const {
+  if (Targets) {
+    LLVM_DEBUG({
----------------
Could reduce indentation by starting with
```
if (!Targets)
  return nullptr;
```

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:175
+                                const LVTypes *Targets) {
+  LLVM_DEBUG({
+    if (References && Targets) {
----------------
Could simplify by doing this first:
```
if (!(References && Targets))
  return;
```

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:201
+LVType *LVType::findIn(const LVTypes *Targets) const {
+  if (Targets) {
+    LLVM_DEBUG({
----------------
Reduce indentation with
```
if (!Targets)
  return nullptr;
```

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

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

https://reviews.llvm.org/D125782