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

[Paul Robinson via Phabricator via llvm-commits]

probinson accepted this revision.
probinson added a comment.
This revision is now accepted and ready to land.

Two nits and LGTM.



================
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));
----------------
I forgot about isa<> when I made my previous comment.  This does less work and is clearer to the reader.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp:111
+  Root = getScopesRoot();
+  EXPECT_NE(Root, nullptr);
+
----------------
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.


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

https://reviews.llvm.org/D125778