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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


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


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


================
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));
+  }
----------------
psamolysov wrote:
> `emplace_back` creates the instance of the `LVPassEntry` class in place.
Replaced with `emplace_back`.


================
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;
----------------
psamolysov wrote:
> 
Nice change.


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


================
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()) << " "
----------------
psamolysov wrote:
> 
Nice change.


================
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;
----------------
psamolysov wrote:
> Just a nit. What about to combine the conditions together by `&&`? I personally think it would be more readable than nested `if` statements.
Replaced the nested `if` with a combined conditions `&&`.


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


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


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


================
Comment at: llvm/unittests/DebugInfo/LogicalView/CompareElementsTest.cpp:68
+    // have the ability of kind selection.
+    T *Element = new T();
+    EXPECT_NE(Element, nullptr);
----------------
psamolysov wrote:
> 
Changed to use the non-throwing overload.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125782