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

[Asher Mancinelli via Phabricator via llvm-commits]

ashermancinelli added inline comments.


================
Comment at: flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp:30-31
+void CrashHandlerFixture::SetUp() {
+  static bool isCrashHanlderRegistered{false};
+  if (not isCrashHanlderRegistered)
+    Fortran::runtime::Terminator::RegisterCrashHandler(CatchCrash);
----------------
awarzynski wrote:
> This is not needed, is it? AFAIK, for every unit tests a new test fixture would be generated anyway.
The terminator's crash handler is static, so I think we only want to register the crash handler once in a translation unit, unless I'm mistaken.


================
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");
+}
----------------
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.


================
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));
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98652