[PATCH] D97403: updated character tests to gtest

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Hi @ashermancinelli ,

Thank you for working on this! I quickly scanned through the test coverage is identical to `flang/unittests/Runtime/character.cpp`. What follows are some ideas for fine-tuning.

- Please don't hesitate to improve test coverage if you see some low hanging fruits :)
- Please remember about file headers <https://llvm.org/docs/CodingStandards.html#file-headers>
- The logic in RuntimeTesting.cpp is a bit complex - is there any chance that that's tested too? (in particular, `CatchCrash`, perhaps by triggering a call to Crash <https://github.com/llvm/llvm-project/blob/19c2e129475013a8a36696d475c9d8681ce52614/flang/runtime/character.cpp#L231>)
- I am also thinking that since procedures from `character.cpp` don't crash (at least the ones that you test), we don't really need `CatchCrash` just yet (though it would be great to see it being tested). Perhaps the test fixture could be simplified?
- IMO we should avoid duplicating code/tests. I recommend deleting similar tests from `flang/unittests/Runtime`.

More comments inline.



================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:17
+  for (std::size_t limit{0}; limit < 8; ++limit) {
+    char x[8];
+    std::size_t xLen{0};
----------------
* this can be declared outside the loop
* both here and in the loop, `8` is a magic number that's re-used and that could be replaced with a variable


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:23
+    RTNAME(CharacterPad1)(x, limit, xLen);
+    ASSERT_LE(xLen, limit) << "xLen " << xLen << ">" << limit;
+    if (x[limit]) {
----------------
IIUC, `CharacterPad1` does not affect `xlen`, so this check could be moved before L22.


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:24-26
+    if (x[limit]) {
+      EXPECT_TRUE(false) << "x[" << limit << "]='" << x[limit] << "'\n";
+      x[limit] = '\0';
----------------
Perhaps:
```
EXPECT_NE(x[limit], `\0`)  << "x[" << limit << "]='" << x[limit] << "'\n";
```
IMHO this would be clearer.


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:28
+    }
+    ASSERT_FALSE(std::memcmp(x, "abcDE   ", limit)) << "x = '" << x << "'";
+  }
----------------
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?


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:47
+  void DoCharacterComparison() {
+    int cmp{RTNAME(CharacterCompareScalar1)(x, y, xBytes, yBytes)};
+    char buf[2][8];
----------------
[nice-to-have] `CharacterCompareScalar2`, `CharacterCompareScalar3`, `CharacterCompareScalar4` :)


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:52
+    std::memcpy(buf[1], y, yBytes);
+    ASSERT_EQ(cmp, expect) << "compare '" << buf[0] << "'(" << xBytes
+                           << ") to '" << buf[1] << "'(" << yBytes << "), got "
----------------
Why not use `x` and `y` instead of `buf[0]` and `buf[1]`, respectively? Doesn't `ASSERT_EQ() <<` behave like `operator<<` for [[ https://en.cppreference.com/w/cpp/io/basic_ostream/operator_ltlt | std::ostream ]]. So there should be no need to convert `x` and `y` to character arrays? Am I missing something?


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:63-67
+TEST_P(CharacterComparisonTestsFixture, CompareCharacters) {
+  DoCharacterComparison();
+  SwapParams();
+  DoCharacterComparison();
+}
----------------
[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!


================
Comment at: flang/unittests/RuntimeGTest/RuntimeTesting.h:17
+// Defines a CHARACTER object with padding when needed
+void SetCharacter(char *, std::size_t, const char *);
+
----------------
This is currently only used in RuntimeTesting.cpp. Perhaps move there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97403