[PATCH] D125781: [llvm-debuginfo-analyzer] 06 - Warning and internal options

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:54
+    };
+    if (const LVScopes *Scopes = Parent->getScopes())
+      for (LVScope *Scope : *Scopes) {
----------------
probinson wrote:
> LLVM style wants braces for this multiline `if` statement.
> https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
Added braces.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:103
+    }
+    exit(0);
+  }
----------------
probinson wrote:
> `exit` in a library function?
This is a good point.

During the development of the CodeView Reader, due to incorrect parsing of the debug information, in some cases the same logical element was added to more than one logical scope.

If this method detects an error, it means the tool will crash as the memory is corrupted: the same logical element being destroyed more than once. That is the reason to force the `exit`.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/WarningInternalTest.cpp:94
+    T *Element = new (std::nothrow) T();
+    EXPECT_NE(Element, nullptr);
+    return Element;
----------------
probinson wrote:
> Use `ASSERT_NE` so the test will abort if the 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/D125781/new/

https://reviews.llvm.org/D125781