[PATCH] D97403: updated character tests to gtest

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:24-26
+    if (x[limit]) {
+      EXPECT_TRUE(false) << "x[" << limit << "]='" << x[limit] << "'\n";
+      x[limit] = '\0';
----------------
ashermancinelli wrote:
> awarzynski wrote:
> > Perhaps:
> > ```
> > EXPECT_NE(x[limit], `\0`)  << "x[" << limit << "]='" << x[limit] << "'\n";
> > ```
> > IMHO this would be clearer.
> I agree, but this should be
> ```
> EXPECT_EQ(x[limit], '\0') << "x[" << limit << "]='" << x[limit] << "'";
> ```
> correct?
+1


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:28
+    }
+    ASSERT_FALSE(std::memcmp(x, "abcDE   ", limit)) << "x = '" << x << "'";
+  }
----------------
ashermancinelli wrote:
> awarzynski wrote:
> > IIUC, at this point x = `abcDE   ` and this `memcpy` simply overwrites `x` with similar value?
> > 
> > I think that [[ https://en.cppreference.com/w/cpp/string/byte/memcpy | std::memcpy ]] either succeeds or we get UB. So this `ASSERT_FALSE` does not really add much, does it?
> `memcmp`, not `memcpy`
:facepalm: Thank you!


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:63-67
+TEST_P(CharacterComparisonTestsFixture, CompareCharacters) {
+  DoCharacterComparison();
+  SwapParams();
+  DoCharacterComparison();
+}
----------------
ashermancinelli wrote:
> awarzynski wrote:
> > [nit] Personally I like to put `ASSERT_` and `EXPECT_` in the main test method (like this one). IMO it makes it much clearer what is actually being tested. But it's a matter of preference!
> I agree, but do you have a suggestion for how to move the assertions/expectations into the body of the test without code repetition due to the parameter swapping? I'm happy to field suggestions on this one :)
IMO this is a borderline situation. Yes, code re-use is nice. But so is making code & tests easier to follow. Which should we prioritize?

Also, I left a comment for `DoCharacterComparison` - I think that that method can be simplified, so there won't be that much code to re-use.

 I marked this as a `[nit]` as IMO it's quite subjective. I'm happy with whatever you decide or propse!


================
Comment at: flang/unittests/RuntimeGTest/RuntimeTesting.h:17
+// Defines a CHARACTER object with padding when needed
+void SetCharacter(char *, std::size_t, const char *);
+
----------------
ashermancinelli wrote:
> awarzynski wrote:
> > This is currently only used in RuntimeTesting.cpp. Perhaps move there?
> Correct, however this will be used in other tests as they are moved over from the old runtime tests so I think it could either remain here or be removed altogether.
Then I suggest removing (so that the patch only contains the required stuff)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97403