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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin marked 2 inline comments as done.
ikudrin added inline comments.


================
Comment at: llvm/unittests/CodeGen/AsmPrinterDwarfTest.cpp:153
+  ASSERT_TRUE(ActualArg0 != nullptr);
+  EXPECT_EQ(&(ActualArg0->getSymbol()), Val.Symbol);
+}
----------------
dblaikie wrote:
> 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.
Let's keep them as is, then.


================
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);
----------------
dblaikie wrote:
> 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)
Ok, will change to `ASSERT_NE(ActualArg0, nullptr)`, this and similar places.

For the record, `ASSERT_TRUE(ActualArg0 != nullptr)` generates
```
.../unittests/CodeGen/AsmPrinterDwarfTest.cpp:176: Failure
Value of: ActualArg0 != nullptr
  Actual: false
Expected: true
```

while `ASSERT_NE(ActualArg0, nullptr)` results in
```
.../unittests/CodeGen/AsmPrinterDwarfTest.cpp:176: Failure
Expected: (ActualArg0) != (nullptr), actual: NULL vs 8-byte object <00-00 00-00 00-00 00-00>
```

which, indeed, seems more legible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87008