[PATCH] D97403: [flang] updated character tests to gtest

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski accepted this revision.
awarzynski added a comment.
This revision is now accepted and ready to land.

LGTM, thank you!



================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:124-126
+    std::memset(buf, 0, sizeof buf);
+    std::memcpy(buf[0], x, xBytes);
+    std::memcpy(buf[1], y, yBytes);
----------------
Btw, I thought that we could get rid of this and now I see that we cannot. My bad, apologies! But I don't think that the code-duplication here is bad. IMO it actually makes this easier to read. But I appreciate that that's subjective :)


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:26
+
+TEST(CharacterTests, AppendAndPad) {
+  static constexpr int limitMax{8};
----------------
ashermancinelli wrote:
> awarzynski wrote:
> > [nit] I would be tempted to separate this and tests for `CharacterCompareScalar{n}` with a big comment, e.g.:
> > ```
> > //-----------------------------------------------------------------------
> > /// Tests (and infra) for CharacterAppend1 and CharacterPad1
> > //------------------------------------------------------------------------
> > ```
> If we're already breaking the tests up into smaller units, would we be better served to break tests into two files?
Both files would be quite small - personally I'd keep it here. But I won't object if you prefer to split.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97403