[PATCH] D98652: [flang] Unittests for runtime terminator

[Andrzej Warzynski via Phabricator via llvm-commits]

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

LGTM, thank you!

As this patch addresses some concerns raised by @klausler  in the review for  D98303 <https://reviews.llvm.org/D98303>,  let's wait at least 24hr before landing this. In case there are some additional comments.



================
Comment at: flang/unittests/RuntimeGTest/RuntimeCrashTest.cpp:64-65
+      buffer, bufferSize, format, std::strlen(format))};
+  ASSERT_DEATH(IONAME(OutputInteger64)(cookie, 0xfeedface),
+      "Unknown 'C' edit descriptor in FORMAT");
+}
----------------
ashermancinelli wrote:
> awarzynski wrote:
> > This would be a bit more idiomatic (helps, a bit, to understand what the magic number is):
> > 
> > ```
> >   ASSERT_DEATH(IONAME(OutputInteger64)(cookie, /*x=*/0xfeedface),
> >       "Unknown 'C' edit descriptor in FORMAT");
> > ```
> > Similar comment in other places.
> IMO, `/*x=*/` does not give any additional information. However I am a fan of the `/*param=*/` style - do you have a suggestion for a more descriptive parameter value to use? Also, in this case, I think the name `OutputInteger64` is clear enough that we don't need the parameter name comment that badly.
I agree and I don't have any good suggestion. Let's leave it as is.


================
Comment at: flang/unittests/RuntimeGTest/RuntimeCrashTest.cpp:128-137
+  IONAME(OutputComplex32)(cookie, 1., 1.);
+  EXPECT_DEATH(IONAME(OutputComplex32)(cookie, 1., 1.),
+      "Internal write overran available records");
+
+  std::memset(buffer, '\0', bufferSize);
+  cookie = IONAME(BeginInternalFormattedOutput)(
+      buffer, bufferSize, format, std::strlen(format));
----------------
ashermancinelli wrote:
> awarzynski wrote:
> > Any chance to make it a bit more visible what the difference between the 2 blocks is? Same elsewhere.
> I left a comment at the top outlining my rationale for performing each IO operation twice - is this sufficient in your opinion?
I was referring to the fact that in e.g. `OverwriteBufferComplexTest` you are testing 2 APIs, whereas in `OverwriteBufferComplexTest` you are testing only one. That wasn't immediately obvious.

This was a nit. If you can improve (e.g. through a comment), that would be great. It's not a blocker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98652