Page MenuHomePhabricator

FileCheck [7/12]: Arbitrary long numeric expressions

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



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.


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

Diff Detail


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
290 ↗(On Diff #205484)

in a legacy

715 ↗(On Diff #205484)

Trailing full stop required.

725–727 ↗(On Diff #205484)

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.

158 ↗(On Diff #205711)

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
553 ↗(On Diff #205484)

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)

198 ↗(On Diff #205484)

/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.

54 ↗(On Diff #208184)

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 ↗(On Diff #208184)

Perhaps "Construct a literal with the specified value."

49 ↗(On Diff #208184)

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.

258 ↗(On Diff #208184)

is always a literal

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

301 ↗(On Diff #208184)

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

722 ↗(On Diff #208184)

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.

17 ↗(On Diff #208184)

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 ↗(On Diff #208184)

const std::set &?

30 ↗(On Diff #208184)

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.

50 ↗(On Diff #208184)

getValue not getvalue.

397 ↗(On Diff #208184)

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.

479 ↗(On Diff #208184)


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
553 ↗(On Diff #205484)

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

722 ↗(On Diff #208184)

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.

25 ↗(On Diff #208184)

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
553 ↗(On Diff #205484)

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

60 ↗(On Diff #208978)

litteral -> literal

49 ↗(On Diff #208978)

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

19 ↗(On Diff #208978)

Why not FileCheckExpressionLiteral Ten(10)?

25 ↗(On Diff #208978)

Why not FileCheckExpressionLiteral Max(...)?

33 ↗(On Diff #208978)

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 ↗(On Diff #208978)


47 ↗(On Diff #208978)

Use std::unordered_set?

grimar added inline comments.Jul 10 2019, 10:03 AM
427 ↗(On Diff #204497)

I'd suggest adding a comment.

47 ↗(On Diff #208978)

Doesn't look like you need this->.

199 ↗(On Diff #208978)

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.

201 ↗(On Diff #208978)

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?

34 ↗(On Diff #208978)

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
201 ↗(On Diff #208978)

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.
199 ↗(On Diff #208978)

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?

33 ↗(On Diff #208978)

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
32 ↗(On Diff #209237)


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.

38 ↗(On Diff #209237)

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
580 ↗(On Diff #209237)

This comment still mentions Expr and SM.

199 ↗(On Diff #208978)

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

199 ↗(On Diff #208978)

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).

32 ↗(On Diff #209237)

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 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
199 ↗(On Diff #208978)

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!

108 ↗(On Diff #209481)

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.
108 ↗(On Diff #209481)

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.
108 ↗(On Diff #209481)

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.

108 ↗(On Diff #209481)

Maybe return VariableProperties{Name, IsPseudo}would work?

jhenderson added inline comments.Jul 12 2019, 8:25 AM
108 ↗(On Diff #209481)

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.