[flang-commits] [flang] 5112bd6 - [flang] Fix a bug in the character runtime

[Diana Picus via flang-commits]

Author: Diana Picus
Date: 2021-05-03T08:08:08Z
New Revision: 5112bd6b6e10b27b81aa83cfdbe3588973a6f1f5

URL: https://github.com/llvm/llvm-project/commit/5112bd6b6e10b27b81aa83cfdbe3588973a6f1f5
DIFF: https://github.com/llvm/llvm-project/commit/5112bd6b6e10b27b81aa83cfdbe3588973a6f1f5.diff

LOG: [flang] Fix a bug in the character runtime

The number of bytes copied in CopyAndPad should depend on the size of
the type being copied, not on its shift value (which in the case of char
is 0, leading to no bytes at all being copied).

Add unit tests for CharacterMin and CharacterMax, which exercise this
code path.

Differential Revision: https://reviews.llvm.org/D101355

Added: 
    

Modified: 
    flang/runtime/character.cpp
    flang/unittests/RuntimeGTest/CharacterTest.cpp

Removed: 
    


################################################################################
diff  --git a/flang/runtime/character.cpp b/flang/runtime/character.cpp
index 3b383bb29ea5c..ed8e5f4984f8b 100644
--- a/flang/runtime/character.cpp
+++ b/flang/runtime/character.cpp
@@ -456,9 +456,9 @@ static void CopyAndPad(
       to[j] = static_cast<TO>(' ');
     }
   } else if (toChars <= fromChars) {
-    std::memcpy(to, from, toChars * shift<TO>);
+    std::memcpy(to, from, toChars * sizeof(TO));
   } else {
-    std::memcpy(to, from, fromChars * shift<TO>);
+    std::memcpy(to, from, fromChars * sizeof(TO));
     for (std::size_t j{fromChars}; j < toChars; ++j) {
       to[j] = static_cast<TO>(' ');
     }

diff  --git a/flang/unittests/RuntimeGTest/CharacterTest.cpp b/flang/unittests/RuntimeGTest/CharacterTest.cpp
index 6afba32e33e35..458a7fd3c8864 100644
--- a/flang/unittests/RuntimeGTest/CharacterTest.cpp
+++ b/flang/unittests/RuntimeGTest/CharacterTest.cpp
@@ -10,6 +10,7 @@
 // in Fortran.
 
 #include "../../runtime/character.h"
+#include "../../runtime/descriptor.h"
 #include "gtest/gtest.h"
 #include <cstring>
 #include <functional>
@@ -134,11 +135,79 @@ TYPED_TEST(CharacterComparisonTests, CompareCharacters) {
     std::memcpy(buf[0], x, xBytes);
     std::memcpy(buf[1], y, yBytes);
     ASSERT_EQ(cmp, expect) << "compare '" << x << "'(" << xBytes << ") to '"
-                           << y << "'(" << yBytes << "), got " << cmp
+                           << y << "'(" << yBytes << "'), got " << cmp
                            << ", should be " << expect << '\n';
   }
 }
 
+// Test MIN() and MAX()
+struct ExtremumTestCase {
+  const char *x, *y, *expect;
+};
+
+template <typename CHAR>
+void RunExtremumTests(const char *which,
+    std::function<void(Descriptor &, const Descriptor &, const char *, int)>
+        function,
+    const std::vector<ExtremumTestCase> &testCases) {
+
+  // Helper for creating, allocating and filling up a descriptor with data from
+  // a raw character literal, converted to the CHAR type used by the test.
+  auto CreateDescriptor = [](const char *raw) -> OwningPtr<Descriptor> {
+    std::size_t length{std::strlen(raw)};
+    std::basic_string<CHAR> converted{raw, raw + length};
+
+    OwningPtr<Descriptor> descriptor{Descriptor::Create(
+        sizeof(CHAR), length, nullptr, 0, nullptr, CFI_attribute_allocatable)};
+    if (descriptor->Allocate() != 0) {
+      return nullptr;
+    }
+
+    std::copy(
+        converted.begin(), converted.end(), descriptor->OffsetElement<CHAR>());
+
+    return descriptor;
+  };
+
+  for (const auto &t : testCases) {
+    OwningPtr<Descriptor> x = CreateDescriptor(t.x);
+    OwningPtr<Descriptor> y = CreateDescriptor(t.y);
+
+    ASSERT_NE(x, nullptr);
+    ASSERT_TRUE(x->IsAllocated());
+    ASSERT_NE(y, nullptr);
+    ASSERT_TRUE(y->IsAllocated());
+    function(*x, *y, nullptr, 0);
+
+    std::basic_string<CHAR> got{
+        x->OffsetElement<CHAR>(), x->ElementBytes() / sizeof(CHAR)};
+    std::basic_string<CHAR> expect{t.expect, t.expect + std::strlen(t.expect)};
+    EXPECT_EQ(expect, got) << which << "('" << t.x << "','" << t.y
+                           << "') for CHARACTER(kind=" << sizeof(CHAR) << ")";
+  }
+}
+
+template <typename CHAR> struct ExtremumTests : public ::testing::Test {};
+TYPED_TEST_CASE(ExtremumTests, CharacterTypes);
+
+TYPED_TEST(ExtremumTests, MinTests) {
+  static std::vector<ExtremumTestCase> tests{
+      {"a", "z", "a"},
+      {"zaaa", "aa", "aa  "},
+      {"aaz", "aaaaa", "aaaaa"},
+  };
+  RunExtremumTests<TypeParam>("MIN", RTNAME(CharacterMin), tests);
+}
+
+TYPED_TEST(ExtremumTests, MaxTests) {
+  static std::vector<ExtremumTestCase> tests{
+      {"a", "z", "z"},
+      {"zaa", "aaaaa", "zaa  "},
+      {"aaaaa", "aazaa", "aazaa"},
+  };
+  RunExtremumTests<TypeParam>("MAX", RTNAME(CharacterMax), tests);
+}
+
 // Test search functions INDEX(), SCAN(), and VERIFY()
 
 template <typename CHAR>