Page MenuHomePhabricator

FileCheck: Return parse error w/ Error & Expected
ClosedPublic

Authored by thopre on Jun 11 2019, 1:38 AM.

Details

Summary

Make use of Error and Expected to bubble up diagnostics and force
checking of errors in the callers.

Diff Detail

Repository
rL LLVM

Event Timeline

thopre created this revision.Jun 11 2019, 1:38 AM

Looks good to me. Just two minor comments:

This adds quite a lot of return make_error<ParseErrorInfo>(SM.GetMessage(SMLoc::getFromPointer(Foo.data()), SourceMgr::DK_Error, "...")));
Maybe adding a helper would make sense? Something like:

Error ParseErrorInfo::get(const SourceManager& SM, const char* LocPtr, const Twine& ErrMsg) {
   return make_error<ParseErrorInfo>(SM.GetMessage(SMLoc::getFromPointer(LocPtr), SourceMgr::DK_Error,  ErrMsg)));
}

Then the above can just be return ParseErrorInfo::get(SM, Foo.data(), "...");

thopre updated this revision to Diff 204238.Jun 12 2019, 2:13 AM
  • Fix bugs in unit test when running with Error checks enabled
  • Make eval() functions return an Expected instead of Optional
  • Create helper functions to create ParseError and its associated diagnostic
  • Rename ParseErrorInfo into FileCheckParseError
thopre marked 3 inline comments as done.Jun 17 2019, 2:29 PM
thopre added inline comments.
llvm/include/llvm/Support/FileCheck.h
351–352 ↗(On Diff #204238)

This can be simplified to Diagnostic.print(nullptr, OS);

llvm/lib/Support/FileCheck.cpp
619–622 ↗(On Diff #204238)

Perhaps worth making match() return an error via an Expected<size_t> return value.

thopre updated this revision to Diff 205358.Jun 18 2019, 8:06 AM
  • Convert match to return Expected as well
  • Simplify FileCheckErrorDiagnostic logging
thopre updated this revision to Diff 205361.Jun 18 2019, 8:29 AM

Fix unit test

thopre updated this revision to Diff 205435.Jun 18 2019, 2:29 PM

Replace assert(false) by llvm_unreachable

thopre updated this revision to Diff 205441.Jun 18 2019, 2:54 PM

Fix code style issues

thopre updated this revision to Diff 205529.Jun 19 2019, 2:15 AM

Remove "represent value" -> "store value" change

This revision is now accepted and ready to land.Jun 19 2019, 7:09 AM
thopre updated this revision to Diff 205687.Jun 19 2019, 3:31 PM
  • Remove useless llvm_unreachable from handleAllErrors
  • Move comment to apply to both ignored errors
This revision was automatically updated to reflect the committed changes.