[flang] Update list-input test to use GTest

[ashermancinelli]

Summary

Update list-input test to use GTest.

[ashermancinelli]

If deemed reasonable, I'm happy to extend this to test more of the api as well - I'm open to suggestions. I'm also happy to keep this patch very short.

flang/unittests/RuntimeGTest/ListInputTest.cpp

32

Buffer names buffer and n should be more descriptive IMO - @awarzynski do you have any suggestions?

[ashermancinelli]

Break original test into two test cases. Add simple sanity-check test cases. Add death test to verify failure message.

[awarzynski]

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.

flang/unittests/RuntimeGTest/ListInputTest.cpp

19

I believe that this method can now be deleted from testing.cpp?

29

[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.

32

IIUC, these are inputBuffers. What do you reckon?

34–37

[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)

35

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.

49

IIUC, these are outputBuffers. Perhaps outputAlphabets would be even better?

60

How about defining const char* expectedOutput = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ " somewhere and using that instead?

62

Update asc.

87

I'd be tempted to be more specific here: are null --> are "Null values" of the Fortran 2018 standard 13.10.3.2

163

What about std::make_tuple("0", std::vector<int>{}),?

[ashermancinelli]

Use more descriptive variable names and better comment tests. Address clang-tidy warnings and phabricator reviews.

[ashermancinelli]

@awarzynski Thank you for the fantastic review comments! I've tried to address most of them - please let me know if this looks good to you.

[awarzynski]

Thank you for addressing my comments @ashermancinelli, this looks really good! One general question, why make everything static? Is there any technical benefit? I feel that it reduces readability a bit. If it's not needed, I suggest removing it.

flang/unittests/RuntimeGTest/CMakeLists.txt

9

Repetition

flang/unittests/RuntimeGTest/ListInputTest.cpp

66

Probably expectedOutput?

93

[nit] Perhaps expectedOutput? want is a bit problematic for me (you may want something, but does imply that you _can_ expect it?). I'm probably overthinking this!

104

wanted have?

ASSERT_EQ(have[j], want[j]) << "want[" << j << "]==" << want[j]
`wanted have`?                                << ", have " << have[j] << '\n';

? (so that what's printed can easily be mapped to what's in the code). Similar comment for other tests.

192

FIXME

[ashermancinelli]

@awarzynski Thanks again for the review! I have another patch ready to address this.

I typically use static to reduce the number of initializations, but you're right to point out that in these tests readability is probably more important than saving a few ms (and most variables are PODs anyways).

flang/unittests/RuntimeGTest/ListInputTest.cpp

66

Could you elaborate? I'm not sure I understand.

93

Fair enough! If it's confusing for you, it'll certainly be confusing to future contributors.

[ashermancinelli]

Use more descriptive variable names.

[klausler]

In D99614#2683137, @ashermancinelli wrote:

@awarzynski Thanks again for the review! I have another patch ready to address this.

I typically use static to reduce the number of initializations, but you're right to point out that in these tests readability is probably more important than saving a few ms (and most variables are PODs anyways).

It's good general practice to use constexpr, static, static const, &c. Gratuitously avoiding good general practice makes code more surprising to a reader who knows C++ and good general practice, and that makes it less readable.

[awarzynski]

In D99614#2683137, @ashermancinelli wrote:

I typically use static to reduce the number of initializations

I was under the false impression that constexpr effectively implied static in this context. I couldn't find a good reference, but here's a small experiment on compiler explorer that demonstrates that constexpr and static constexpr are indeed different: https://godbolt.org/z/9qh16zeGc (adding static makes a a compile-time constant). There's no difference when compiling with optimisations enabled (e.g.-O1). Apologies for the confusion.

@klausler , do you prefer static to be restored here? I'm fine with this patch either way so accepting as is.

@ashermancinelli LGTM, thank you for working on this! Could you add missing newline characters before merging?

flang/unittests/RuntimeGTest/ListInputTest.cpp

66

I'm re-rereading this now and see what you meant. Makes sense!

[ashermancinelli]

Merged in ccef0adc5968.

flang/unittests/Runtime/CMakeLists.txt

target_link_libraries(external-hello-world
   FortranRuntime
 )
 
 add_flang_nongtest_unittest(external-io
   RuntimeTesting
   FortranRuntime
 )
 
-add_flang_nongtest_unittest(list-input
-  RuntimeTesting
-  FortranRuntime
-)
-
 add_flang_nongtest_unittest(buffer
   RuntimeTesting
   FortranRuntime
 )

flang/unittests/Runtime/list-input.cpp

-// Basic sanity tests for list-directed input
-
-#include "testing.h"
-#include "../../runtime/descriptor.h"
-#include "../../runtime/io-api.h"
-#include "../../runtime/io-error.h"
-#include <algorithm>
-#include <cstring>
-
-using namespace Fortran::runtime;
-using namespace Fortran::runtime::io;
-
-int main() {
-  StartTests();
-
-  char buffer[4][32];
-  int j{0};
-  for (const char *p : {"1 2 2*3  ,", ",6,,8,1*",
-           "2*'abcdefghijklmnopqrstuvwxyzABC", "DEFGHIJKLMNOPQRSTUVWXYZ'"}) {
-    SetCharacter(buffer[j++], sizeof buffer[0], p);
-  }
-  for (; j < 4; ++j) {
-    SetCharacter(buffer[j], sizeof buffer[0], "");
-  }
-
-  StaticDescriptor<1> staticDescriptor;
-  Descriptor &whole{staticDescriptor.descriptor()};
-  SubscriptValue extent[]{4};
-  whole.Establish(TypeCode{CFI_type_char}, sizeof buffer[0], &buffer, 1, extent,
-      CFI_attribute_pointer);
-  whole.Dump();
-  whole.Check();
-
-  try {
-    auto cookie{IONAME(BeginInternalArrayListInput)(whole)};
-    std::int64_t n[9]{-1, -2, -3, -4, 5, -6, 7, -8, 9};
-    std::int64_t want[9]{1, 2, 3, 3, 5, 6, 7, 8, 9};
-    for (j = 0; j < 9; ++j) {
-      IONAME(InputInteger)(cookie, n[j]);
-    }
-    char asc[2][54]{};
-    IONAME(InputAscii)(cookie, asc[0], sizeof asc[0] - 1);
-    IONAME(InputAscii)(cookie, asc[1], sizeof asc[1] - 1);
-    if (auto status{IONAME(EndIoStatement)(cookie)}) {
-      Fail() << "list-directed input failed, status "
-             << static_cast<int>(status) << '\n';
-    } else {
-      for (j = 0; j < 9; ++j) {
-        if (n[j] != want[j]) {
-          Fail() << "wanted n[" << j << "]==" << want[j] << ", got " << n[j]
-                 << '\n';
-        }
-      }
-      for (j = 0; j < 2; ++j) {
-        if (std::strcmp(asc[j],
-                "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ ") != 0) {
-          Fail() << "wanted asc[" << j << "]=alphabets, got '" << asc[j]
-                 << "'\n";
-        }
-      }
-    }
-  } catch (const std::string &crash) {
-    Fail() << "crash: " << crash << '\n';
-  }
-
-  return EndTests();
-}

flang/unittests/Runtime/testing.h

 #ifndef FORTRAN_TEST_RUNTIME_TESTING_H_
 #define FORTRAN_TEST_RUNTIME_TESTING_H_
 
 #include "llvm/Support/raw_ostream.h"
 #include <cstddef>
 
 namespace llvm {
 class raw_ostream;
 }
 
 void StartTests();
 llvm::raw_ostream &Fail();
 int EndTests();
 
-// Defines a CHARACTER object with padding when needed
-void SetCharacter(char *, std::size_t, const char *);
-
 #endif // FORTRAN_TEST_RUNTIME_TESTING_H_

flang/unittests/Runtime/testing.cpp

 int EndTests() {
   if (failures == 0) {
     llvm::outs() << "PASS\n";
   } else {
     llvm::outs() << "FAIL " << failures << " tests\n";
   }
   return failures != 0;
 }
-
-void SetCharacter(char *to, std::size_t n, const char *from) {
-  auto len{std::strlen(from)};
-  std::memcpy(to, from, std::min(len, n));
-  if (len < n) {
-    std::memset(to + len, ' ', n - len);
-  }
-}

flang/unittests/RuntimeGTest/CMakeLists.txt

 add_flang_unittest(FlangRuntimeTests
   CharacterTest.cpp
   CrashHandlerFixture.cpp
   MiscIntrinsic.cpp
   Numeric.cpp
   NumericalFormatTest.cpp
   Reduction.cpp
   RuntimeCrashTest.cpp
+  ListInputTest.cpp
 )
 
 target_link_libraries(FlangRuntimeTests
   PRIVATE
   FortranRuntime
 )

flang/unittests/RuntimeGTest/ListInputTest.cpp

+//===-- flang/unittests/RuntimeGTest/ListInputTest.cpp ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CrashHandlerFixture.h"
+#include "../../runtime/descriptor.h"
+#include "../../runtime/io-api.h"
+#include "../../runtime/io-error.h"
+
+using namespace Fortran::runtime;
+using namespace Fortran::runtime::io;
+
+// Pads characters with whitespace when needed
+void SetCharacter(char *to, std::size_t n, const char *from) {
+  auto len{std::strlen(from)};
+  std::memcpy(to, from, std::min(len, n));
+  if (len < n) {
+    std::memset(to + len, ' ', n - len);
+  }
+}
+
+struct InputTest : CrashHandlerFixture {};
+
+TEST(InputTest, TestListInputAlphabet) {
+  constexpr int numInputBuffers{2};
+  constexpr int maxInputBufferLength{32};
+  char inputBuffers[numInputBuffers][maxInputBufferLength];
+  const char expectedOutput[]{
+      "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ "};
+  int j{0};
+
+  // Use _two_ input buffers and _three_ output buffers. Note the `3*` in the
+  // _inputBuffers_.
+  SetCharacter(inputBuffers[j++], maxInputBufferLength,
+      "3*'abcdefghijklmnopqrstuvwxyzABC");
+  SetCharacter(
+      inputBuffers[j++], maxInputBufferLength, "DEFGHIJKLMNOPQRSTUVWXYZ'");
+
+  StaticDescriptor<1> staticDescriptor;
+  Descriptor &whole{staticDescriptor.descriptor()};
+  SubscriptValue extent[]{numInputBuffers};
+  whole.Establish(TypeCode{CFI_type_char}, maxInputBufferLength, &inputBuffers,
+      1, extent, CFI_attribute_pointer);
+  whole.Check();
+  auto *cookie{IONAME(BeginInternalArrayListInput)(whole)};
+
+  constexpr int numOutputBuffers{3};
+  constexpr int outputBufferLength{54};
+  char outputBuffers[numOutputBuffers][outputBufferLength]{};
+  for (j = 0; j < numOutputBuffers; ++j) {
+    IONAME(InputAscii)(cookie, outputBuffers[j], outputBufferLength - 1);
+  }
+
+  const auto status{IONAME(EndIoStatement)(cookie)};
+  ASSERT_EQ(status, 0) << "list-directed input failed, status "
+                       << static_cast<int>(status) << '\n';
+
+  // Verify results that the _two_ ascii inputs result in _three_ alphabets
+  for (j = 0; j < numOutputBuffers; ++j) {
+    ASSERT_EQ(std::strcmp(outputBuffers[j], expectedOutput), 0)
+        << "wanted outputBuffers[" << j << "]=" << expectedOutput << ", got '"
+        << outputBuffers[j] << "'\n";
+  }
+}
+
+TEST(InputTest, TestListInputIntegerList) {
+  constexpr int numBuffers{2};
+  constexpr int maxBufferLength{32};
+  char buffer[numBuffers][maxBufferLength];
+  int j{0};
+  SetCharacter(buffer[j++], maxBufferLength, "1 2 2*3  ,");
+  SetCharacter(buffer[j++], maxBufferLength, ",6,,8,1*");
+
+  StaticDescriptor<1> staticDescriptor;
+  Descriptor &whole{staticDescriptor.descriptor()};
+  SubscriptValue extent[]{numBuffers};
+  whole.Establish(TypeCode{CFI_type_char}, maxBufferLength, &buffer, 1, extent,
+      CFI_attribute_pointer);
+  whole.Check();
+  auto *cookie{IONAME(BeginInternalArrayListInput)(whole)};
+
+  constexpr int listInputLength{9};
+
+  // Negative numbers will be overwritten by _expectedOutput_, and positive
+  // numbers will not be as their indices are "Null values" of the Fortran 2018
+  // standard 13.10.3.2 in the format strings _buffer_
+  std::int64_t actualOutput[listInputLength]{-1, -2, -3, -4, 5, -6, 7, -8, 9};
+  const std::int64_t expectedOutput[listInputLength]{1, 2, 3, 3, 5, 6, 7, 8, 9};
+  for (j = 0; j < listInputLength; ++j) {
+    IONAME(InputInteger)(cookie, actualOutput[j]);
+  }
+
+  const auto status{IONAME(EndIoStatement)(cookie)};
+  ASSERT_EQ(status, 0) << "list-directed input failed, status "
+                       << static_cast<int>(status) << '\n';
+
+  // Verify the calls to _InputInteger_ resulted in _expectedOutput_
+  for (j = 0; j < listInputLength; ++j) {
+    ASSERT_EQ(actualOutput[j], expectedOutput[j])
+        << "wanted actualOutput[" << j << "]==" << expectedOutput[j] << ", got "
+        << actualOutput[j] << '\n';
+  }
+}
+
+TEST(InputTest, TestListInputInvalidFormatWithSingleSuccess) {
+  std::string formatBuffer{"1, g"};
+  constexpr int numBuffers{1};
+
+  StaticDescriptor<1> staticDescriptor;
+  Descriptor &whole{staticDescriptor.descriptor()};
+  SubscriptValue extent[]{numBuffers};
+  whole.Establish(TypeCode{CFI_type_char}, formatBuffer.size(),
+      formatBuffer.data(), 1, extent, CFI_attribute_pointer);
+  whole.Check();
+
+  auto *cookie{IONAME(BeginInternalArrayListInput)(whole)};
+  std::int64_t dummy;
+
+  // Perform _InputInteger_ once successfully
+  IONAME(InputInteger)(cookie, dummy);
+
+  // Perform failing InputInteger
+  ASSERT_DEATH(IONAME(InputInteger)(cookie, dummy),
+      "Bad character 'g' in INTEGER input field");
+}
+
+// Same test as _TestListInputInvalidFormatWithSingleSuccess_, however no
+// successful call to _InputInteger_ is performed first.
+TEST(InputTest, TestListInputInvalidFormat) {
+  std::string formatBuffer{"g"};
+  constexpr int numBuffers{1};
+
+  StaticDescriptor<1> staticDescriptor;
+  Descriptor &whole{staticDescriptor.descriptor()};
+  SubscriptValue extent[]{numBuffers};
+  whole.Establish(TypeCode{CFI_type_char}, formatBuffer.size(),
+      formatBuffer.data(), 1, extent, CFI_attribute_pointer);
+  whole.Check();
+
+  auto *cookie{IONAME(BeginInternalArrayListInput)(whole)};
+  std::int64_t dummy;
+
+  // Perform failing InputInteger
+  ASSERT_DEATH(IONAME(InputInteger)(cookie, dummy),
+      "Bad character 'g' in INTEGER input field");
+}
+
+using ParamTy = std::tuple<std::string, std::vector<int>>;
+
+struct SimpleListInputTest : testing::TestWithParam<ParamTy> {};
+
+TEST_P(SimpleListInputTest, TestListInput) {
+  auto [formatBuffer, expectedOutput] = GetParam();
+  constexpr int numBuffers{1};
+
+  StaticDescriptor<1> staticDescriptor;
+  Descriptor &whole{staticDescriptor.descriptor()};
+  SubscriptValue extent[]{numBuffers};
+  whole.Establish(TypeCode{CFI_type_char}, formatBuffer.size(),
+      formatBuffer.data(), 1, extent, CFI_attribute_pointer);
+  whole.Check();
+  auto *cookie{IONAME(BeginInternalArrayListInput)(whole)};
+
+  const auto listInputLength{expectedOutput.size()};
+  std::vector<std::int64_t> actualOutput(listInputLength);
+  for (std::size_t j = 0; j < listInputLength; ++j) {
+    IONAME(InputInteger)(cookie, actualOutput[j]);
+  }
+
+  const auto status{IONAME(EndIoStatement)(cookie)};
+  ASSERT_EQ(status, 0) << "list-directed input failed, status "
+                       << static_cast<int>(status) << '\n';
+
+  // Verify the calls to _InputInteger_ resulted in _expectedOutput_
+  for (std::size_t j = 0; j < listInputLength; ++j) {
+    ASSERT_EQ(actualOutput[j], expectedOutput[j])
+        << "wanted actualOutput[" << j << "]==" << expectedOutput[j] << ", got "
+        << actualOutput[j] << '\n';
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(SimpleListInputTestInstantiation, SimpleListInputTest,
+    testing::Values(std::make_tuple("", std::vector<int>{}),
+        std::make_tuple("0", std::vector<int>{}),
+        std::make_tuple("1", std::vector<int>{1}),
+        std::make_tuple("1, 2", std::vector<int>{1, 2}),
+        std::make_tuple("3*2", std::vector<int>{2, 2, 2})), );
 No newline at end of file