Page MenuHomePhabricator

FileCheck [7/12]: Arbitrary long numeric expressions
ClosedPublic

Authored by thopre on Apr 7 2019, 4:21 PM.

Details

Summary

This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch extend numeric expression to
support an arbitrary number of operands, either variable or literals.

Copyright:

  • Linaro (changes up to diff 183612 of revision D55940)
  • GraphCore (changes in later versions of revision D55940 and in new revision created off D55940)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jhenderson added inline comments.Jun 19 2019, 7:45 AM
llvm/lib/Support/FileCheck.cpp
320

in a legacy

737–738

Trailing full stop required.

737–745

I think this is actually unnecessary. handleAllErrors terminates the program with an llvm_unreachable anyway if there are any unhandled errors.

thopre updated this revision to Diff 205711.Jun 19 2019, 4:49 PM

Make new function parameter immediate use the /*ParamName=*/Value code style

thopre marked an inline comment as done.Jun 19 2019, 4:52 PM

I think I am happy with this, leaving the rest up to James.

llvm/include/llvm/Support/FileCheck.h
162

I prefer the description of the return value to give the expected/normal case first, and the error case afterward. So,

\returns the expression value, or an error ...
thopre updated this revision to Diff 208099.Jul 4 2019, 4:53 PM
thopre marked 17 inline comments as done.
  • Rebase on top of refactoring patches
  • Address comments
llvm/include/llvm/Support/FileCheck.h
499

I've used it in other place already. I'm using "against" because the diagnostic relates to the buffer held by SM (it is created via SM.GetMessage)

llvm/lib/Support/FileCheck.cpp
219

/There is yes. See patch 9/12

thopre updated this revision to Diff 208151.Jul 5 2019, 4:58 AM

Rebase on latest changes in patch series

thopre updated this revision to Diff 208184.Jul 5 2019, 7:35 AM

Rebase on top of latest changes in patch series

Mostly looks good. A few nits still and one or two more significant comments. I think this structure looks much cleaner than the previous version, personally.

llvm/include/llvm/Support/FileCheck.h
54

As things stand, this only can represent an unsigned literal. I think you'll want to support negative literals or unary negation operands fairly quickly. Otherwise, the expression "-1" doesn't work.

I'm also slightly concerned in the current form that somebody could try to use FileCheckExpressionLiteral for negative numbers, and get surprising results. Perhaps worth updating the comment to say that the class represents an unsigned integer literal.

60

Perhaps "Construct a literal with the specified value."

llvm/lib/Support/FileCheck.cpp
65

Might be worth making this comment more generic e.g. "Handle a failed operation, e.g. due to an undefined variable." That'll make the comment robust to other possible future failure types.

275–276

is always a literal

The comment should probably start with "The second..."

319–339

The comment should probably start with "The first..."

737–738

You've changed this message implying it can have more than one undefined variable, but I don't see any lit test case change that has more than one. You should add/update one.

llvm/unittests/Support/FileCheckTest.cpp
17

I think it would be a good idea to have a test case showing that this can take std::numeric_limits<uint64_t>::max().

25

const std::set &?

30

Perhaps worth having this print what names are still remaining if not empty. I think you can do something like EXPECT_TRUE(ExpectedUndefVarNames.empty()) << ExpectedUndefVarNames.

48

getValue not getvalue.

409

In other places you've used bool(SubstValue) for this sort of conversion. I think that might be nicer, but don't care that much. Regardless, it should be consistent throughout.

529–530

unique_ptr?

thopre updated this revision to Diff 208975.Jul 10 2019, 9:00 AM
thopre marked 15 inline comments as done.

Address review comments.

thopre added inline comments.Jul 10 2019, 9:00 AM
llvm/include/llvm/Support/FileCheck.h
499

Is my explanation satisfying or do you want to use another phrasing?

llvm/lib/Support/FileCheck.cpp
737–738

There was a unit test already but it doesn't test that message indeed. I've added 1 lit test and updated another one, the former for correct substitution and the latter for undefined variables.

llvm/unittests/Support/FileCheckTest.cpp
25

This cannot be const since erase is called on that set. Making it a reference means the set needs to be created in all the caller when now the caller can provide an initializer as is done in expectUndefError below.

thopre updated this revision to Diff 208978.Jul 10 2019, 9:02 AM

Run clang-format-diff

jhenderson added inline comments.Jul 10 2019, 9:34 AM
llvm/include/llvm/Support/FileCheck.h
60

litteral -> literal

499

Yes, it's fine. Not the phrasing I would use but I don't care much about it.

llvm/lib/Support/FileCheck.cpp
65

eg. -> e.g.
either "an undefined variable" or "undefined variables"

llvm/unittests/Support/FileCheckTest.cpp
19

Why not FileCheckExpressionLiteral Ten(10)?

25

Why not FileCheckExpressionLiteral Max(...)?

33

Other instances of overloading operator<< in the LLVM unit tests tend to just be in the llvm namespace, not in std. See for example ADT/StringRefTest.cpp.

The variables in this function don't use LLVM naming style.

35

StringRef?

47

Use std::unordered_set?

grimar added inline comments.Jul 10 2019, 10:03 AM
llvm/lib/Support/FileCheck.cpp
63

Doesn't look like you need this->.

206

btw it was a bit confusing for me to find there is a method that take bool IsPseudo
and method that take bool &IsPseudo, i.e. that one of them can change this flag, but another - not.

208

You can avoid having this temorarily variable:

if (ParseVarResult)
  return parseNumericVariableUse(*ParseVarResult, IsPseudo, SM);

But actually it seems you also need to check the result of Expected,
not sure why it is assumed there is no error?

459

I'd suggest adding a comment.

llvm/unittests/Support/FileCheckTest.cpp
34

Seems a different variables naming style used here? Ten, Value vs first, set. You need stick to one I think.
(To upper case I guess? LLVM did not do a switch to a new one yet, except LLD project I believe).

grimar added inline comments.Jul 10 2019, 10:05 AM
llvm/lib/Support/FileCheck.cpp
208

Oh, sorry, I misread the code, you actually check it. Then you still can avoid having the temp var :)

thopre marked 16 inline comments as done.Jul 11 2019, 8:53 AM
thopre added inline comments.
llvm/lib/Support/FileCheck.cpp
206

You mean parseVariable Vs parseNumericVariableUse? Would you rather me have both be reference even though the latter only needs to get the value? Or is there a naming convention to denote a reference as opposed to a parameter passed by value?

llvm/unittests/Support/FileCheckTest.cpp
33

I'm not sure why but when I try with llvm namespace and I switch to unordered_set as suggested below the overloaded operator does not get used and the code fails to compile. However the code compiles and works with llvm namespace if I use a set instead of unordered_set.

I've thus decided to just write a setToString method and output the result of calling it. It does create lots of temporaries though.

thopre updated this revision to Diff 209237.Jul 11 2019, 8:54 AM
thopre marked an inline comment as done.

Address review comments

jhenderson added inline comments.Jul 12 2019, 1:20 AM
llvm/unittests/Support/FileCheckTest.cpp
31

static?

Though saying that, this is inside an anonymous namespace, so perhaps the other functions need static removing.

This function returns a StringRef, yet the base type is a local variable. I think that will result in the memory being freed under the reference. Perhaps better would be to make Str a Twine and then use Twine::str() to return a std::string.

37

Perhaps make this ", ", and start and end the full string with '{'/'}' to make it easier to read.

grimar added inline comments.Jul 12 2019, 2:15 AM
llvm/include/llvm/Support/FileCheck.h
485–492

This comment still mentions Expr and SM.

llvm/lib/Support/FileCheck.cpp
206

There is no special naming conventions, but given that parseVariable method always must set IsPseudo flag,
but another parse* methods can only read the flags they are given, I wonder if it be better/cleaner for parseVariable to return Expected<std::pair<StringRef, bool> for example?

thopre updated this revision to Diff 209456.Jul 12 2019, 3:59 AM
thopre marked 7 inline comments as done.

Address review comments

llvm/lib/Support/FileCheck.cpp
206

I'm not a big fan of pairs because its members are accessed without a name (first or second) which I find it less readable. Anyway, done but I've reverted the change to make the call to parseNumericVariableUse a one liner so that it is clear what is being passed to that function (ParseVarResult->first is not very descriptive).

llvm/unittests/Support/FileCheckTest.cpp
31

Ah yes indeed, the StringRef does not own the string it references. I cannot use a Twine for Str because you cannot store a Twine so while I can do Str + S, I cannot store the result in Str (see https://llvm.org/doxygen/classllvm_1_1Twine.html#a28b850ffd65d5997cf730d644e06d89b I've been bitten by this before in this patchset). I guess I'll return a std::string then. I've used a Twine to do the " " concatenation though.

jhenderson added inline comments.Jul 12 2019, 5:19 AM
llvm/lib/Support/FileCheck.cpp
206

You could use a small struct to name the members, perhaps called VariableProperties:

struct VariableProperties {
  StringRef Name;
  bool IsPseudo;
}
thopre updated this revision to Diff 209481.Jul 12 2019, 7:37 AM
thopre marked an inline comment as done.

Return a VariableProperties structure in parseVariable

jhenderson accepted this revision.Jul 12 2019, 7:47 AM

LGTM. Thanks for the hard work on this!

llvm/lib/Support/FileCheck.cpp
122

You might be able to just do return {Name, IsPseudo} to avoid the temporary.

This revision is now accepted and ready to land.Jul 12 2019, 7:47 AM
thopre marked 2 inline comments as done.Jul 12 2019, 8:04 AM
thopre added inline comments.
llvm/lib/Support/FileCheck.cpp
122

I tried but it didn't work. I think it would if I was returning the structure, but here I'm returning an Expected.

As per 6.6.3 C++11 draft:
"A return statement with a braced-init-list initializes the object or reference to be returned from the function by copy-list-initialization (8.5.4) from the specified initializer list."

So it tries to build an Expected with that initialization list which fails because I think an Expected<foo> only accepts a Foo.

thopre marked 2 inline comments as done.Jul 12 2019, 8:08 AM
thopre added inline comments.
llvm/lib/Support/FileCheck.cpp
122

Yes, that's it:

/home/thomasp/repos/llvm-project/llvm/lib/Support/FileCheck.cpp:107:10: error: no matching constructor for initialization of 'Expected<FileCheckPattern::VariableProperties>'

return {Name, IsPseudo};
       ^~~~~~~~~~~~~~~~

/home/thomasp/repos/llvm-project/llvm/include/llvm/Support/Error.h:474:3: note: candidate template ignored: requirement 'std::is_convertible<StringRef &, VariableProperties>::value' was not satisfied [with OtherT = llvm::StringRef &]

Expected(OtherT &&Val,
^

It tries to convert Name into a VariableProperties and fails. All the other constructors expect an Error or another Expected and fail as well.

Thanks for your work on this! Looking forward to be able to use these new features the next time I update our fork.

llvm/lib/Support/FileCheck.cpp
122

Maybe return VariableProperties{Name, IsPseudo}would work?

jhenderson added inline comments.Jul 12 2019, 8:25 AM
llvm/lib/Support/FileCheck.cpp
122

Yes of course. I think you could still do it inline if you want, (not tried, so not 100% certain off the top of my head) and don't mind either way:

return VariableProperties {Name, IsPseudo};
thopre updated this revision to Diff 209680.Jul 13 2019, 1:11 AM

Return structure in parseVariable as a temporary

thopre marked 2 inline comments as done.Jul 13 2019, 1:12 AM
This revision was automatically updated to reflect the committed changes.