FileCheck [7/12]: Arbitrary long numeric expressions

[thopre]

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)

[jhenderson]

llvm/lib/Support/FileCheck.cpp

318

in a legacy

736–737

Trailing full stop required.

736–744

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

[thopre]

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

[probinson]

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

llvm/include/llvm/Support/FileCheck.h

146

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]

• Rebase on top of refactoring patches
• Address comments

llvm/include/llvm/Support/FileCheck.h

513

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

224

/There is yes. See patch 9/12

[thopre]

Rebase on latest changes in patch series

[thopre]

Rebase on top of latest changes in patch series

[jhenderson]

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.

278–279

is always a literal

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

340

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

736–737

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.

53

getValue not getvalue.

407

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.

541–542

unique_ptr?

[thopre]

Address review comments.

[thopre]

llvm/include/llvm/Support/FileCheck.h

513

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

llvm/lib/Support/FileCheck.cpp

736–737

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]

Run clang-format-diff

[jhenderson]

llvm/include/llvm/Support/FileCheck.h

60

litteral -> literal

513

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]

llvm/lib/Support/FileCheck.cpp

63

Doesn't look like you need this->.

211

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.

213

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?

458

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]

llvm/lib/Support/FileCheck.cpp

213

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

[thopre]

llvm/lib/Support/FileCheck.cpp

211

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]

Address review comments

[jhenderson]

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]

llvm/include/llvm/Support/FileCheck.h

504–507

This comment still mentions Expr and SM.

llvm/lib/Support/FileCheck.cpp

211

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]

Address review comments

llvm/lib/Support/FileCheck.cpp

211

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]

llvm/lib/Support/FileCheck.cpp

211

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

struct VariableProperties {
  StringRef Name;
  bool IsPseudo;
}

[thopre]

Return a VariableProperties structure in parseVariable

[jhenderson]

LGTM. Thanks for the hard work on this!

llvm/lib/Support/FileCheck.cpp

124

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

[thopre]

llvm/lib/Support/FileCheck.cpp

124

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]

llvm/lib/Support/FileCheck.cpp

124

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.

[arichardson]

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

124

Maybe return VariableProperties{Name, IsPseudo}would work?

[jhenderson]

llvm/lib/Support/FileCheck.cpp

124

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]

Return structure in parseVariable as a temporary