Page MenuHomePhabricator

[flang] Update list-input test to use GTest
ClosedPublic

Authored by ashermancinelli on Mar 30 2021, 11:36 AM.

Details

Summary

Update list-input test to use GTest.

Diff Detail

Event Timeline

ashermancinelli requested review of this revision.Mar 30 2021, 11:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2021, 11:36 AM

If deemed reasonable, I'm happy to extend this to test more of the api as well - I'm open to suggestions. I'm also happy to keep this patch very short.

flang/unittests/RuntimeGTest/ListInputTest.cpp
32

Buffer names buffer and n should be more descriptive IMO - @awarzynski do you have any suggestions?

Break original test into two test cases. Add simple sanity-check test cases. Add death test to verify failure message.

Hi @ashermancinelli , many thanks for this patch. All tests from the original file are covered and there are also some new tests!

I've left a few comments/suggestions, but nothing major.

flang/unittests/RuntimeGTest/ListInputTest.cpp
19

I believe that this method can now be deleted from testing.cpp?

29

[nit] I would be tempted to define the input, generated output and the expected output somewhere at the top. That's the key info here, everything else is just implementation details. Similar point for other tests.

32

IIUC, these are inputBuffers. What do you reckon?

34–37

[nit] I would be tempted to simplify this as:

SetCharacter(inputBuffers[0], maxBufferLength, "2*'abcdefghijklmnopqrstuvwxyzABC");
SetCharacter(inputBuffers[1], maxBufferLength, "DEFGHIJKLMNOPQRSTUVWXYZ'");

(there's really no need for a loop here)

35

If we replace 2* with 3* then there will be 2 input buffers and 3 output buffers. IMHO that would make this test more interesting and emphasize the split between input and output a bit better.

49

IIUC, these are outputBuffers. Perhaps outputAlphabets would be even better?

60

How about defining const char* expectedOutput = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ " somewhere and using that instead?

62

Update asc.

87

I'd be tempted to be more specific here: are null --> are "Null values" of the Fortran 2018 standard 13.10.3.2

163

What about std::make_tuple("0", std::vector<int>{}),?

Use more descriptive variable names and better comment tests. Address clang-tidy warnings and phabricator reviews.

ashermancinelli marked 10 inline comments as done.Apr 9 2021, 4:54 PM

@awarzynski Thank you for the fantastic review comments! I've tried to address most of them - please let me know if this looks good to you.

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.

flang/unittests/RuntimeGTest/CMakeLists.txt
9

Repetition

flang/unittests/RuntimeGTest/ListInputTest.cpp
65

Probably expectedOutput?

92

[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!

103

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.

191

FIXME

ashermancinelli added a comment.EditedApr 12 2021, 8:27 AM

@awarzynski Thanks again for the review! I have another patch ready to address this.

I typically use static to reduce the number of initializations, but you're right to point out that in these tests readability is probably more important than saving a few ms (and most variables are PODs anyways).

flang/unittests/RuntimeGTest/ListInputTest.cpp
65

Could you elaborate? I'm not sure I understand.

92

Fair enough! If it's confusing for you, it'll certainly be confusing to future contributors.

Use more descriptive variable names.

@awarzynski Thanks again for the review! I have another patch ready to address this.

I typically use static to reduce the number of initializations, but you're right to point out that in these tests readability is probably more important than saving a few ms (and most variables are PODs anyways).

It's good general practice to use constexpr, static, static const, &c. Gratuitously avoiding good general practice makes code more surprising to a reader who knows C++ and good general practice, and that makes it less readable.

awarzynski accepted this revision.Apr 13 2021, 3:00 AM

I typically use static to reduce the number of initializations

I was under the false impression that constexpr effectively implied static in this context. I couldn't find a good reference, but here's a small experiment on compiler explorer that demonstrates that constexpr and static constexpr are indeed different: https://godbolt.org/z/9qh16zeGc (adding static makes a a compile-time constant). There's no difference when compiling with optimisations enabled (e.g.-O1). Apologies for the confusion.

@klausler , do you prefer static to be restored here? I'm fine with this patch either way so accepting as is.

@ashermancinelli LGTM, thank you for working on this! Could you add missing newline characters before merging?

flang/unittests/RuntimeGTest/ListInputTest.cpp
65

I'm re-rereading this now and see what you meant. Makes sense!

This revision is now accepted and ready to land.Apr 13 2021, 3:00 AM