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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


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


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


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



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125782