Page MenuHomePhabricator

Method loadFromCommandLine should be able to report errors
ClosedPublic

Authored by sepavloff on May 17 2017, 5:10 AM.

Details

Summary

Now FixedCompilationDatabase::loadFromCommandLine has no means to report
which error occurred if it fails to create compilation object. This is
a block for implementing D33013, because after that change driver refuses
to create compilation if command line contains erroneous options.

This change adds additional argument to loadFromCommandLine, which is
assigned error message text if compilation object was not created. This is
the same way as other methods of CompilationDatabase report failure.

Diff Detail

Repository
rL LLVM

Event Timeline

sepavloff created this revision.May 17 2017, 5:10 AM
joerg added a subscriber: joerg.May 18 2017, 9:38 AM

Can this use ErrorOr?

Can this use ErrorOr?

It makes sense if constructing methods of CompilationDatabase were also changed to return ErrorOr, as there is no need to have different interfaces for the similar functionality. On the other hand, such change would not present a gain over existing implementation IMHO.

sepavloff updated this revision to Diff 99600.May 19 2017, 11:10 AM

Change loadFromCommandLine so that it returns std::unique_ptr as methods of CompilationDatabase does

alexfh accepted this revision.May 22 2017, 6:27 AM

Thank you! This looks good with one nit (see the inline comment).

lib/Tooling/CompilationDatabase.cpp
292 ↗(On Diff #99600)

The lack of the arguments (or the -- below) shouldn't be treated as an error (at least an error that is worth showing to the user). The caller will just move on to trying the next kind of a compilation database. The only actual errors we can get here may be coming from stripPositionalArgs.

The caller should be modified accordingly (i.e. not output anything when both the return value is nullptr and ErrorMsg is empty).

This revision is now accepted and ready to land.May 22 2017, 6:27 AM
alexfh added inline comments.May 22 2017, 6:31 AM
lib/Tooling/CompilationDatabase.cpp
164 ↗(On Diff #99600)

Another couple of nits:

  1. the comment below would probably be better moved to the body of the second if
  2. enclose the next statement in braces.
sepavloff updated this revision to Diff 99786.May 22 2017, 10:14 AM
sepavloff marked an inline comment as done.

Addressed reviewer's notes.

Thank you!

I put updated fix here. If it is OK, I'll commit it tomorrow.

lib/Tooling/CompilationDatabase.cpp
292 ↗(On Diff #99600)

What if error message contains a prefix that indicates if this is error or warning? Errors could be printed and warnings ignored in CommonOptionsParser.cpp:122. Such solution would allow clients to investigate failures in more details.

alexfh added inline comments.May 23 2017, 3:23 AM
lib/Tooling/CommonOptionsParser.cpp
122 ↗(On Diff #99786)

Getting into the business of encoding the type of error into the string is not what we want in this particular case ;] Let's just check whether ErrorMessage is empty (we could go further and make it an Optional<std::string>, but an empty string seems to be equally reasonable here).

lib/Tooling/CompilationDatabase.cpp
292 ↗(On Diff #99600)

I don't think it's valuable in this specific code. I'd just return nullptr without touching ErrorMsg and change the caller to only print something when the ErrorMsg is not empty.

The lack of the -- in the command line is not an error, it's just a way to automatically find a compilation database from the file system (which is probably even more frequently used feature than the fixed compilation database, so we definitely should not treat this case as an error).

I see your reasons. Updated the patch accordingly.
Thank you!

sepavloff updated this revision to Diff 99919.May 23 2017, 8:30 AM

Updated patch

alexfh accepted this revision.May 23 2017, 8:41 AM

Thanks, LG!

This revision was automatically updated to reflect the committed changes.