[PATCH] D99614: [flang] Update list-input test to use GTest

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Hi @ashermancinelli , many thanks for this patch. All tests from the original file are covered and there are also some new tests!

I've left a few comments/suggestions, but nothing major.



================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:18
+// Pads characters with whitespace when needed
+void SetCharacter(char *to, std::size_t n, const char *from) {
+  auto len{std::strlen(from)};
----------------
I believe that this method can now be deleted from testing.cpp?


================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:28
+
+TEST(InputTest, TestListInputAlphabet) {
+  static constexpr int numBuffers{2};
----------------
[nit] I would be tempted to define the input, generated output and the expected output somewhere at the top. That's the key info here, everything else is just implementation details. Similar point for other tests.


================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:31
+  static constexpr int maxBufferLength{32};
+  static char buffer[numBuffers][maxBufferLength];
+  int j{0};
----------------
IIUC, these are `inputBuffers`. What do you reckon?


================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:33-36
+  for (const char *p :
+      {"2*'abcdefghijklmnopqrstuvwxyzABC", "DEFGHIJKLMNOPQRSTUVWXYZ'"}) {
+    SetCharacter(buffer[j++], maxBufferLength, p);
+  }
----------------
[nit] I would be tempted to simplify this as:
```
  SetCharacter(inputBuffers[0], maxBufferLength, "2*'abcdefghijklmnopqrstuvwxyzABC");
  SetCharacter(inputBuffers[1], maxBufferLength, "DEFGHIJKLMNOPQRSTUVWXYZ'");
```
(there's really no need for a loop here)


================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:34
+  for (const char *p :
+      {"2*'abcdefghijklmnopqrstuvwxyzABC", "DEFGHIJKLMNOPQRSTUVWXYZ'"}) {
+    SetCharacter(buffer[j++], maxBufferLength, p);
----------------
If we replace `2*` with `3*` then there will be 2 input buffers and 3 output buffers. IMHO that would make this test more interesting and emphasize the split between input and output a bit better.


================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:48
+  static constexpr int inputBufferLength{54};
+  static char inputBuffers[numInputBuffers][inputBufferLength]{};
+  IONAME(InputAscii)(cookie, inputBuffers[0], inputBufferLength - 1);
----------------
IIUC, these are `outputBuffers`. Perhaps `outputAlphabets` would be even better?


================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:59
+    ASSERT_EQ(std::strcmp(inputBuffers[j],
+                  "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ "),
+        0)
----------------
How about defining `const char* expectedOutput = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ "` somewhere and using that instead?


================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:61
+        0)
+        << "wanted asc[" << j << "]=alphabets, got '" << inputBuffers[j]
+        << "'\n";
----------------
Update `asc`.


================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:86
+  // Negative numbers will be overwritten by _want_, and positive numbers will
+  // not be as their indices are null in the format strings _buffer_
+  static std::int64_t have[listInputLength]{-1, -2, -3, -4, 5, -6, 7, -8, 9};
----------------
I'd be tempted to be more specific here: `are null` --> `are "Null values" of the Fortran 2018 standard 13.10.3.2`


================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:162
+    testing::Values(std::make_tuple("", std::vector<int>{}),
+        std::make_tuple("1", std::vector<int>{1}),
+        std::make_tuple("1, 2", std::vector<int>{1, 2}),
----------------
What about `std::make_tuple("0", std::vector<int>{}),`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99614