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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thank you for working on this!

This is a very neat validation of the crash handling API. It also demonstrates that the GTest API in no way obfuscates that crash output. In other words, this shows that we can rely on `Fortran::runtime::Terminator::RegisterCrashHandler` in other GTest unit tests.

I've left a few comments inline.

[nit] Could you clarify somewhere _why_ testing these particular APIs?

Otherwise, LGTM!



================
Comment at: flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp:30-31
+void CrashHandlerFixture::SetUp() {
+  static bool isCrashHanlderRegistered{false};
+  if (not isCrashHanlderRegistered)
+    Fortran::runtime::Terminator::RegisterCrashHandler(CatchCrash);
----------------
This is not needed, is it? AFAIK, for every unit tests a new test fixture would be generated anyway.


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


================
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));
----------------
Any chance to make it a bit more visible what the difference between the 2 blocks is? Same elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98652