[PATCH] D87008: [DebugInfo] Fix methods of AsmPrinter to emit values corresponding to the DWARF format (1/19).

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp:153
+  ASSERT_TRUE(ActualArg0 != nullptr);
+  EXPECT_EQ(&(ActualArg0->getSymbol()), Val.Symbol);
+}
----------------
ikudrin wrote:
> dblaikie wrote:
> > Ekxtra parens look like they're unnecessary here?
> They are added only to improve readability. They are not required for the compiler, sure. Do you prefer them to be removed?
Marginally prefer not to have them - but no harm if you prefer them there, your call.


================
Comment at: llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp:176
+  const MCSymbolRefExpr *ActualArg0 = dyn_cast_or_null<MCSymbolRefExpr>(Arg0);
+  ASSERT_TRUE(ActualArg0 != nullptr);
+  EXPECT_EQ(&(ActualArg0->getSymbol()), Val.Symbol);
----------------
ikudrin wrote:
> dblaikie wrote:
> > Is there an assert non-null you could use more directly?
> Apart from more exotic variants, we can use `ASSERT_TRUE(ActualArg0)`, `ASSERT_TRUE(ActualArg0 != nullptr)` or `ASSERT_NE(ActualArg0, nullptr)` here. I have chosen a variant based on my understanding of expressibility. What is your preference?
Looks like the LLVM unittests use NE/EQ about 4:1, so maybe switch to that? (makes for slightly more legible error generally, though probably no major difference for nullptr testing)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87008