[flang-commits] [flang] [llvm] [Flang] Introduce *Value classes with unittests (PR #216958)

[Eugene Epshteyn via flang-commits]

================
@@ -0,0 +1,257 @@
+//===-- include/flang/Evaluate/character-value-impl.h -----------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef FORTRAN_EVALUATE_CHARACTER_VALUE_IMPL_H_
+#define FORTRAN_EVALUATE_CHARACTER_VALUE_IMPL_H_
+
+#include "flang/Evaluate/common.h"
+#include "llvm/Support/ErrorHandling.h"
+#include <cstddef>
+#include <optional>
+#include <string>
+#include <utility>
+#include <variant>
+
+namespace Fortran::evaluate::value {
+
+class CharacterValueImpl {
+  using Storage =
+      std::variant<std::monostate, std::string, std::u16string, std::u32string>;
+
+public:
+  // rule-of-five
+  ~CharacterValueImpl() = default;
+  CharacterValueImpl(const CharacterValueImpl &) = default;
+  CharacterValueImpl(CharacterValueImpl &&) = default;
+  CharacterValueImpl &operator=(const CharacterValueImpl &) = default;
+  CharacterValueImpl &operator=(CharacterValueImpl &&) = default;
+
+  CharacterValueImpl() = default;
+  explicit CharacterValueImpl(int kind, std::string s) {
+    withCharProto(kind, [&](auto c) {
+      using CharT = std::decay_t<decltype(c)>;
+      using StringT = std::basic_string<CharT>;
+      if (std::is_same_v<StringT, std::string>) {
+        storage_ = std::move(s);
+      } else {
+        StringT buf;
+        buf.resize(s.length());
+        for (auto [i, c] : llvm::enumerate(s)) {
+          buf[i] = c;
----------------
eugeneepshteyn wrote:

`c` here is `char`, which is **signed** on x86-64 but **unsigned** on AArch64, PowerPC and s390x. So for any byte ≥ 0x80 this widening sign-extends on some hosts and not others, and the same CHARACTER value ends up folded differently depending on the machine flang was built on — which cuts against the host-independent folding contract in `flang/docs/Intrinsics.md`. Flang's existing `ICHAR` implementation guards the identical case explicitly (`flang/lib/Evaluate/character.h`: *"Convert first to an unsigned integer type to avoid sign extension"*).

Measured against this PR built on x86-64 (all four should be `0x00000080`):

```console
CharacterValue(2, "\x80")[0]            got 0x0000FF80   want 0x00000080
CharacterValue(4, "\x80")[0]            got 0xFFFFFF80   want 0x00000080
CharacterValue(1, "\x80")[0]            got 0xFFFFFF80   want 0x00000080
(CharacterValue(4, "") += '\x80')[0]    got 0xFFFFFF80   want 0x00000080
```

On an AArch64 host the same four print `0x00000080` and pass.

```suggestion
          buf[i] = static_cast<unsigned char>(c);
```

Two sibling sites need the same treatment — I verified the fix yields `0x00000080` on both `char` ABIs at each:

- `flang/lib/Evaluate/character-value-impl.cpp`, `CharacterValueImpl::operator[]`: `return static_cast<char32_t>(s[i]);` → `static_cast<char32_t>(static_cast<unsigned char>(s[i]))` (only for the `std::string` alternative; `char16_t`/`char32_t` are already unsigned).
- `flang/lib/Evaluate/character-value-impl.cpp`, `operator+=(char)`: `s.push_back(static_cast<CharT>(c));` → widen `c` through `unsigned char` first.

Worth noting for the test as well: `CharacterValueTest.cpp`'s `SubscriptWidensToChar32` writes its expectation as `char32_t('\x80')`, which is itself ABI-dependent — it evaluates to `0xFFFFFF80` on x86-64 and `0x00000080` on AArch64. Both sides of the comparison move together, so the test passes on either host while asserting two different things, and it would keep passing if the widening regressed. An explicit code point pins it:

```c++
TEST(CharacterValue, SubscriptWidensToChar32) {
  CharacterValue u{1, std::string{"\x80"}};
  EXPECT_EQ(char32_t{0x80}, u[0]);

  CharacterValue w{2, std::u16string{char16_t{0x100}}};
  EXPECT_EQ(char32_t{0x100}, w[0]);

  CharacterValue v{4, std::u32string{U"\U0001F600"}};
  EXPECT_EQ(char32_t{0x1F600}, v[0]);
}
```

and the kind-1 → kind-2/4 conversion is worth a case of its own, since nothing currently covers a non-ASCII byte through the constructor:

```c++
TEST(CharacterValue, WidensHighBytesWithoutSignExtension) {
  const std::string high{static_cast<char>(0x80)};
  EXPECT_EQ(std::u16string{u"€"}, *CharacterValue(2, high).AsU16String());
  EXPECT_EQ(std::u32string{U"\U00000080"}, *CharacterValue(4, high).AsU32String());
}
```


https://github.com/llvm/llvm-project/pull/216958