[PATCH] D33272: Method loadFromCommandLine should be able to report errors

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.


================
Comment at: lib/Tooling/CommonOptionsParser.cpp:122
+      FixedCompilationDatabase::loadFromCommandLine(argc, argv, ErrorMessage);
+  if (!Compilations && StringRef(ErrorMessage).startswith("error"))
+    llvm::errs() << ErrorMessage;
----------------
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).


================
Comment at: lib/Tooling/CompilationDatabase.cpp:292
+  if (Argc == 0) {
+    ErrorMsg = "error: no arguments specified\n";
+    return nullptr;
----------------
sepavloff wrote:
> alexfh wrote:
> > 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).
> 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.
> 
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).


https://reviews.llvm.org/D33272