[PATCH] D97703: [AsmParser][SystemZ][z/OS] Introducing HLASM Comment Syntax

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp:69
+    TheTarget = getTarget(TripleName);
+    EXPECT_NE(TheTarget, nullptr);
+
----------------
anirudhp wrote:
> scott.linder wrote:
> > I think these should be `ASSERT_NE`, because the rest of the test relies on this
> I do see your point, because if the returned pointers are null, it doesn't make sense to run the rest of the test.
> 
> In terms of the ASSERT_* macros, I don't believe you can use them in a constructor. I could create a separate function where these are checked and call this for each individual test fixture.
> 
> But looking at some of the other tests that currently create an AsmParser, MCAsmInfo instances, either the returned pointer isn't checked, or its checked using the EXPECT_* macro. So I'm leaning towards just keeping it like this (for now anyway).
I didn't realize the `ASSERT_*` macros weren't available; I am fine with the `EXPECT_*` macros, as they will still correctly flag the test as a failure, there just may be additional noise in the output when the test fails. If we are already doing this elsewhere I am happy to say LGTM!


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

https://reviews.llvm.org/D97703