[PATCH] D125783: [llvm-debuginfo-analyzer] 08 - ELF Reader

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Finished my review pass on this patch.



================
Comment at: llvm/test/tools/llvm-debuginfo-analyzer/DWARF/04-dwarf-missing-nested-enumerators.test:24
+; references to the enumerators 'RED' and 'BLUE'. The DWARF generated
+; by GCC, CodeView generated by Clang and MSVC, it does include such
+; references.
----------------



================
Comment at: llvm/test/tools/llvm-debuginfo-analyzer/DWARF/05-dwarf-incorrect-lexical-scope-variable.test:25
+
+; In the following logical views, we can see that the CodeView debug
+; information generated by the Clang compiler shows the variables
----------------
I'm not seeing a CodeView display in this test.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/ELFReaderTest.cpp:80
+  const LVLocations *Ranges = CompileUnit->getRanges();
+  EXPECT_NE(Ranges, nullptr);
+  ASSERT_EQ(Ranges->size(), 1);
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > It could make sense to check pointers on the `nullptr` in `ASSERT_NE` instead of `EXPECT_NE`. If a pointer is null, the test will just crash on the first dereference of the pointer.
> That is a good point.
> I believe a function that uses `ASSERT_NE` must be declared with `TEST` or `TEST_F` and I can't see how to return a value, which it is our case.
> In order to use `ASSERT_NE` then we need to change how those functions are implemented and invoked.
> 
There are other ASSERT_* macros used in this function, so I don't think your concern is correct.


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

https://reviews.llvm.org/D125783