[PATCH] D125778: [llvm-debuginfo-analyzer] 03 - Logical elements

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h:87
+  void setReference(LVElement *Element) override {
+    assert((!Element || dyn_cast<LVSymbol>(Element)) && "Invalid element");
+    setReference(static_cast<LVSymbol *>(Element));
----------------
probinson wrote:
> I forgot about isa<> when I made my previous comment.  This does less work and is clearer to the reader.
Changed `dyn_cast` to `isa`.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp:111
+  Root = getScopesRoot();
+  EXPECT_NE(Root, nullptr);
+
----------------
probinson wrote:
> CarlosAlbertoEnciso wrote:
> > probinson wrote:
> > > I think `ASSERT_NE` here too.
> > `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.
> createElements does return `void` so ASSERT_NE can be used here.
Good point. Changed to `ASSERT_NE`.


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

https://reviews.llvm.org/D125778