[PATCH] D97403: updated character tests to gtest
Andrzej Warzynski via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 25 11:58:58 PST 2021
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
More information about the llvm-commits
mailing list