Page MenuHomePhabricator

FileCheck [3/12]: Stricter parsing of @LINE expressions
ClosedPublic

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

Details

Summary

This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch gives earlier and better
diagnostics for the @LINE expressions.

Rather than detect parsing errors at matching time, this commit adds
enhance parsing to detect issues with @LINE expressions at parse time
and diagnose them more accurately.

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)

Diff Detail

Repository
rL LLVM

Event Timeline

thopre created this revision.Apr 7 2019, 4:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2019, 4:18 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
thopre updated this revision to Diff 194153.Apr 8 2019, 8:38 AM

Use camel case for new and modified functions

thopre updated this revision to Diff 194476.Apr 10 2019, 4:42 AM
  • Fix detection of unexpected whitespace in @LINE expressions
  • Make numeric value constructor explicit
arsenm added a subscriber: arsenm.Apr 11 2019, 12:33 PM
arsenm added inline comments.
llvm/lib/Support/FileCheck.cpp
95 ↗(On Diff #194476)

I thought these used Twine, not std::string?

tra removed a subscriber: tra.Apr 11 2019, 1:15 PM
thopre marked an inline comment as done.Apr 12 2019, 4:05 AM
thopre added inline comments.
llvm/lib/Support/FileCheck.cpp
95 ↗(On Diff #194476)

It is but there is an implicit conversion from the latter to the former. Using Twine directly makes the diagnostic string less readable due to akward concatenation:

Twine("Unsupported numeric operation '").concat(Twine(Operator)).concat("'")

I feel the current code provides better readability.

probinson added inline comments.Apr 12 2019, 6:26 AM
llvm/lib/Support/FileCheck.cpp
67 ↗(On Diff #194780)

I'd use normal comments not doxygen for a small static helper like this.

75 ↗(On Diff #194780)

Please put doxygen for new APIs in the header not the implementation file. (Eventually the doxygen for all the methods should be in the header, but that's for a later cleanup.)

thopre updated this revision to Diff 195131.Apr 15 2019, 4:32 AM

Add unit testing

thopre marked 2 inline comments as done.Apr 15 2019, 4:34 AM
arichardson added inline comments.Apr 16 2019, 7:26 AM
llvm/lib/Support/FileCheck.cpp
60 ↗(On Diff #195131)

I guess this should be S

95 ↗(On Diff #194476)

I think StringRef("Unsupported numeric operation '") + Operator + "'" should also work and won't create a copy for the std::string.

thopre updated this revision to Diff 195539.Apr 17 2019, 4:36 AM
thopre marked 3 inline comments as done.

Concatenate diagnostic message via StringRef and Twine rather than std::string

thopre updated this revision to Diff 195754.Apr 18 2019, 8:39 AM
  • Fix codestyle issues
  • Avoid concatenation when printing diagnostics
thopre updated this revision to Diff 196265.Apr 23 2019, 9:02 AM

Rebase on top of latest changes

This revision is now accepted and ready to land.Apr 24 2019, 7:47 AM
jhenderson added inline comments.Apr 29 2019, 7:16 AM
llvm/lib/Support/FileCheck.cpp
60 ↗(On Diff #196265)

next is a very unspecific function name to me, and given this is quite a large file, a slightly more specific name might be clearer, e.g. "popFront" or similar.

llvm/unittests/Support/FileCheckTest.cpp
100 ↗(On Diff #196265)

parseExpect?

thopre updated this revision to Diff 197109.Apr 29 2019, 7:31 AM
thopre marked 2 inline comments as done.
  • Rename next to popFront
  • Rename parse_expect to parseExpect to follow naming convention
jhenderson accepted this revision.Apr 29 2019, 8:43 AM
This revision was automatically updated to reflect the committed changes.
rnk added a comment.Apr 29 2019, 12:53 PM

The newly added test fails on Windows:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/6381

This appears to be the fix:

bool parseExpect(std::string &VarName, std::string &Trailer) {
  • StringRef NameTrailer = StringRef(VarName + Trailer);

+ std::string NameTrailer = VarName + Trailer;

In D60383#1482917, @rnk wrote:

The newly added test fails on Windows:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/6381

This appears to be the fix:

bool parseExpect(std::string &VarName, std::string &Trailer) {
  • StringRef NameTrailer = StringRef(VarName + Trailer); + std::string NameTrailer = VarName + Trailer;

Sadly I cannot reproduce it. I tried a stage2 build just in case but I still don't hit it. I'm also a bit confused by what is happening. Does the temporary std::string created by the + operator dies before NameTrailer is used? Anyway, if your patch works could you try std::string &NameTrailer = VarName + Trailer instead? I believe that removes an unnecessary copy from the temporary std::string created by the + operator to the NameTrailer one.

Thanks for the patch!

In D60383#1482917, @rnk wrote:

The newly added test fails on Windows:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/6381

This appears to be the fix:

bool parseExpect(std::string &VarName, std::string &Trailer) {
  • StringRef NameTrailer = StringRef(VarName + Trailer); + std::string NameTrailer = VarName + Trailer;

Sadly I cannot reproduce it. I tried a stage2 build just in case but I still don't hit it. I'm also a bit confused by what is happening. Does the temporary std::string created by the + operator dies before NameTrailer is used? Anyway, if your patch works could you try std::string &NameTrailer = VarName + Trailer instead? I believe that removes an unnecessary copy from the temporary std::string created by the + operator to the NameTrailer one.

Thanks for the patch!

Oh I'm always doing Debug builds so probably at -O0 the std::string is actually created and stays valid when it's copied in the memory buffer while in a release build the compiler might not even create the temporary concatenated string or destroys it quickly.

In D60383#1482917, @rnk wrote:

The newly added test fails on Windows:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/6381

This appears to be the fix:

bool parseExpect(std::string &VarName, std::string &Trailer) {
  • StringRef NameTrailer = StringRef(VarName + Trailer); + std::string NameTrailer = VarName + Trailer;

Sadly I cannot reproduce it. I tried a stage2 build just in case but I still don't hit it. I'm also a bit confused by what is happening. Does the temporary std::string created by the + operator dies before NameTrailer is used? Anyway, if your patch works could you try std::string &NameTrailer = VarName + Trailer instead? I believe that removes an unnecessary copy from the temporary std::string created by the + operator to the NameTrailer one.

Thanks for the patch!

Oh sorry realized the patch is already committed and yes making it a reference does not work. Thanks Reid!