This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck, unittest] Improve readability of ExpressionFormat
ClosedPublic

Authored by thopre on Jun 18 2020, 4:22 PM.

Details

Summary

Factor out repetetitive code into helper function and split massive
ExpressionFormat method test into separate test for each method,
removing dead code in passing. Also add a MinInt64 and MaxInt64 checks
when testing getMatchingString.

Diff Detail

Event Timeline

thopre created this revision.Jun 18 2020, 4:22 PM
jhenderson accepted this revision.Jun 19 2020, 12:27 AM

Thanks, it's a big diff, but I think I followed it and am happy with the new logic (I didn't try to make sure it matched the old stuff, but I don't think that's strictly necessarily in this case). LGTM, but maybe give it a few days to give others a chance to chime in if they want.

This revision is now accepted and ready to land.Jun 19 2020, 12:27 AM
grimar accepted this revision.Jun 19 2020, 1:42 AM

LG. Few suggestions inlined.

llvm/unittests/Support/FileCheckTest.cpp
83

This can be constexpr.

108

Perhaps the following reads slightly better?

if (!AllowHex) {
  MaxUint64Str = std::to_string(MaxUint64);
  MaxInt64Str = std::to_string(MaxInt64);
  MinInt64Str = std::to_string(MinInt64);
  TenStr = "10";
  FifteenStr = "15";
  FirstInvalidCharDigits = "aA";
  AcceptedHexOnlyDigits = RefusedHexOnlyDigits = "N/A";
  return;
}

MaxUint64Str = AllowUpperHex ? "FFFFFFFFFFFFFFFF" : "ffffffffffffffff";
MaxInt64Str = AllowUpperHex ? "7FFFFFFFFFFFFFFF" : "7fffffffffffffff";
TenStr = AllowUpperHex ? "A" : "a";
FifteenStr = AllowUpperHex ? "F" : "f";
AcceptedHexOnlyDigits = AllowUpperHex ? "ABCDEF" : "abcdef";
RefusedHexOnlyDigits = AllowUpperHex ? "abcdef" : "ABCDEF";
342–343

Probably move these 2 lines too to have all declarations in a single place?

thopre marked 3 inline comments as done.Jun 19 2020, 6:45 AM

Thanks, it's a big diff, but I think I followed it and am happy with the new logic (I didn't try to make sure it matched the old stuff, but I don't think that's strictly necessarily in this case). LGTM, but maybe give it a few days to give others a chance to chime in if they want.

I split the test and then rewrote progressively check by check so nothing should be missing. I've only removed the InvalidTenStr bit because it was not actually tested. There is a test for G which already test invalid char hex digit, which I think came when I realized it was simpler because it didn't require an if condition. So as much testing as before and a bit more (INT64_MIN and INT64_MAX)

thopre updated this revision to Diff 272051.Jun 19 2020, 6:46 AM

Address review comments

LG. Few suggestions inlined.

Better indeed, now it's almost neutral line wise with 2 new tests and generally much more readable code:

1 file changed, 156 insertions(+), 153 deletions(-)

This revision was automatically updated to reflect the committed changes.