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)

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

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

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

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

75

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
66

I guess this should be S

95

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
66

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!