This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Allow parenthesized expressions
ClosedPublic

Authored by arichardson on Apr 3 2020, 5:15 AM.

Details

Summary

With this change it is be possible to write FileCheck expressions such
as #(VAR+1)-2. Currently, the only supported arithmetic operators are
plus and minus, so this is not particularly useful yet. However, it our
CHERI fork we have tests that benefit from having multiplication in
FileCheck expressions. Allowing parenthesized expressions is the simplest
way for us to work around the current lack of operator precedence in
FileCheck expressions.

Diff Detail

Event Timeline

arichardson created this revision.Apr 3 2020, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 5:15 AM
thopre added a comment.Apr 3 2020, 6:33 AM

Should there be some tests under llvm/test/FileCheck as well? Perhaps in numeric-expressions.txt?

llvm/lib/Support/FileCheckImpl.h
662–669

Maybe mentions parenthesized expression here?

Thanks for this. I think this is a good step in the right direction for FileCheck's numeric expression support. You should probably add something to the FileCheck documentation about operator precedence and also using parentheses.

llvm/unittests/Support/FileCheckTest.cpp
747

Do you think it might make sense for a test case for more closing parentheses than opening ones? E.g. ")" or "())" or maybe even "())("?

Perhaps also something like "(1)(2)" and "2(X)" too with no operation between the operands?

arichardson marked 2 inline comments as done.
  • rebase on latest master
  • add suggested test checks
  • Mention parenthesized expressions in FileCheck.rst

I haven't added tests to numeric-expressions.txt in this revision. Do you think the unit tests are sufficient?

I haven't added tests to numeric-expressions.txt in this revision. Do you think the unit tests are sufficient?

Yes to test end-to-end. I think 2 tests would be enough: (i) one positive and one negative test. No need to repeat all the variants of positive and negative tests that you did in the unit test.

jhenderson accepted this revision.May 27 2020, 1:58 AM

LGTM, with a few nits and once thopre's comments have been addressed. Approving now as I'm off tomorrow, so if you get them done before I'm back, I don't want to hold things up.

llvm/docs/CommandGuide/FileCheck.rst
697

This sentence reads slightly clunkily to me. How about "however, parentheses" -> "but"?

llvm/lib/Support/FileCheckImpl.h
668

Shouldn't it be \p Expr?

llvm/unittests/Support/FileCheckTest.cpp
755–756

If you plan on improving the situation, perhaps move the second sentence to a TODO comment on the next line.

This revision is now accepted and ready to land.May 27 2020, 1:58 AM
  • add numeric-expression.txt test
arichardson marked 3 inline comments as done.
  • Address latest review comments
arichardson added inline comments.May 27 2020, 2:14 AM
llvm/unittests/Support/FileCheckTest.cpp
755–756

I don't have any immediate plans since I have lots of other things on my TODO list.

jhenderson added inline comments.May 27 2020, 2:50 AM
llvm/test/FileCheck/numeric-expression.txt
167 ↗(On Diff #266458)

Nit: too many blank lines

179–180 ↗(On Diff #266458)

Are you able to put these on the same lines as the value themselves, like in the other examples above?

PAREN EXPRESSIONS: // PAREN-OP-LABEL: PAREN EXPRESSIONS
11 // PAREN-OP-NEXT: [[#(NUMVAR+2)-1]]
11 // PAREN-OP-NEXT: [[#NUMVAR+(2-1)]]
arichardson marked 2 inline comments as done.
  • improve test.
  • really fix test
jhenderson accepted this revision.May 27 2020, 3:58 AM

LGTM. as before.

thopre accepted this revision.May 27 2020, 6:43 AM

LGTM, thanks! I'll adapt the testcase once there's signed values to show precedence by using a variable using max uint64_t and once there's multiplication operand we can use that instead.

This revision was automatically updated to reflect the committed changes.