[PATCH] D125782: [llvm-dva] 07 - Compare elements

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added a comment.

Good job! I have a few most stylistic only comments here. What I would like to discuss with the other reviewers: should an `assert` be used in every function that accepts an argument by pointer and dereferences the pointer or calls functions on it (so, dereferences it too)? My personal opinion: yes but there can be other opinions too, so I do not insist.



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h:28
+
+class LVCompare {
+  raw_ostream &OS;
----------------
Either the class should be marked as `final` or has a virtual destructor.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h:62
+  static LVCompare &getInstance();
+  static void setInstance(LVCompare *Compare);
+
----------------
I see a call to this static member function in the `LVCompare::execute` only. Could the static member function be `private` or `protected`?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h:72
+  void addPassEntry(LVReader *Reader, LVElement *Element, LVComparePass Pass) {
+    PassTable.push_back(LVPassEntry(Reader, Element, Pass));
+  }
----------------
`emplace_back` creates the instance of the `LVPassEntry` class in place.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVCompare.cpp:67
+  // In case the same reader instance is used.
+  for (LVCompareInfo::value_type &Entry : Results) {
+    std::get<getExpected()>(Entry.second) = 0;
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVCompare.cpp:212-214
+                ScopeLinks.insert(
+                    std::make_pair(static_cast<LVScope *>(CurrentTarget),
+                                   static_cast<LVScope *>(Reference)));
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVCompare.cpp:287
+      dbgs() << "\nReference/Target Scope links:\n";
+      for (const LVScopeLink::value_type &Entry : ScopeLinks)
+        dbgs() << "Source: " << hexSquareString(Entry.first->getOffset()) << " "
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:869-870
+      (options().getCompareLines() && lineCount() != Scope->lineCount()))
+    return false;
+  return true;
+}
----------------
Could the method just return the inverted `if`'s condition instead of using the `if` statement?


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:876
+    if (Container)
+      for (const auto &Entry : *Container)
+        Entry->setIsInCompare();
----------------
Is `Entry` a reference to a pointer?


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1054
+LVScope *LVScopeAggregate::equals(const LVScopes *Scopes) const {
+  for (LVScope *Scope : *Scopes)
+    if (equals(Scope))
----------------
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." 


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1779-1780
+  // When comparing logical elements, ignore any difference in the children.
+  if (options().getCompareContext())
+    if (!equalNumberOfChildren(Scope))
+      return false;
----------------
Just a nit. What about to combine the conditions together by `&&`? I personally think it would be more readable than nested `if` statements.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1804-1805
+
+  if (getReference())
+    if (!getReference()->equals(Scope->getReference()))
+      return false;
----------------
Just a nit. What about to combine the conditions together by `&&`? I personally think it would be more readable than nested `if` statements.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1937-1938
+
+  if (getReference())
+    if (!getReference()->equals(Scope->getReference()))
+      return false;
----------------
Just a nit. What about to combine the conditions together by `&&`? I personally think it would be more readable than nested `if` statements.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp:363-364
+
+  if (getReference())
+    if (!getReference()->equals(Symbol->getReference()))
+      return false;
----------------
Just a nit. What about to combine the conditions together by `&&`? I personally think it would be more readable than nested if statements.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:515-518
+  if ((getTypeName() != Type->getTypeName()) || (getName() != Type->getName()))
+    return false;
+
+  return true;
----------------
I believe this `if` statement can be converted to return of the inverted condition.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/CompareElementsTest.cpp:68
+    // have the ability of kind selection.
+    T *Element = new T();
+    EXPECT_NE(Element, nullptr);
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125782