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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


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


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


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


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


================
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) {
----------------
probinson wrote:
> 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."
> 
`During comparison notify the Reader of the new element.`
`


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1054
+LVScope *LVScopeAggregate::equals(const LVScopes *Scopes) const {
+  for (LVScope *Scope : *Scopes)
+    if (equals(Scope))
----------------
probinson wrote:
> 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.
Good point. Added to the improvements list.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/CompareElementsTest.cpp:69
+    T *Element = new (std::nothrow) T();
+    EXPECT_NE(Element, nullptr);
+    (Element->*Function)();
----------------
probinson wrote:
> Use `ASSERT_NE` so the test will abort if allocation fails.
`ASSERT_NE` can be used only inside functions that don't return a value.
 
See https://google.github.io/googletest/advanced.html#assertion-placement

> The one constraint is that assertions that generate a fatal failure (FAIL* and ASSERT_*) can only be used in void-returning functions.


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

https://reviews.llvm.org/D125782