[PATCH] D99614: [flang] Update list-input test to use GTest
Andrzej Warzynski via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 12 03:37:48 PDT 2021
awarzynski added a comment.
Thank you for addressing my comments @ashermancinelli, this looks really good! One general question, why make everything `static`? Is there any technical benefit? I feel that it reduces readability a bit. If it's not needed, I suggest removing it.
================
Comment at: flang/unittests/RuntimeGTest/CMakeLists.txt:9
RuntimeCrashTest.cpp
+ CrashHandlerFixture.cpp
+ ListInputTest.cpp
----------------
Repetition
================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:65
+ ASSERT_EQ(std::strcmp(outputBuffers[j], expectedOutput), 0)
+ << "wanted outputBuffers[" << j << "]=" << expectedOutput << ", got '"
+ << outputBuffers[j] << "'\n";
----------------
Probably `expectedOutput`?
================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:92
+ static std::int64_t have[listInputLength]{-1, -2, -3, -4, 5, -6, 7, -8, 9};
+ static const std::int64_t want[listInputLength]{1, 2, 3, 3, 5, 6, 7, 8, 9};
+ for (j = 0; j < listInputLength; ++j) {
----------------
[nit] Perhaps `expectedOutput`? `want` is a bit problematic for me (you may `want` something, but does imply that you _can_ `expect` it?). I'm probably overthinking this!
================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:103
+ for (j = 0; j < listInputLength; ++j) {
+ ASSERT_EQ(have[j], want[j]) << "wanted have[" << j << "]==" << want[j]
+ << ", got " << have[j] << '\n';
----------------
`wanted have`?
```
ASSERT_EQ(have[j], want[j]) << "want[" << j << "]==" << want[j]
`wanted have`? << ", have " << have[j] << '\n';
```
? (so that what's printed can easily be mapped to what's in the code). Similar comment for other tests.
================
Comment at: flang/unittests/RuntimeGTest/ListInputTest.cpp:191
+ std::make_tuple("3*2", std::vector<int>{2, 2, 2})), );
\ No newline at end of file
----------------
FIXME
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99614/new/
https://reviews.llvm.org/D99614
More information about the llvm-commits
mailing list