[clang-query] Continue if compilation command not found for some files

[george.karpenkov]

Summary

This is a somewhat controversial change, but I have found it very convenient when running clang-query on entire projects.

---

When searching for a code pattern in an entire project with a compilation database it's tempting to run

clang-query *.cpp

And yet, that often breaks because some files are just not in the compilation database: tests, sample code, etc.
I don't think clang-query should stop when encountering such cases.

[ilya-biryukov]

The change LG, but maybe also exit with an error code whenever some of the ASTs couldn't be built?

clang-tools-extra/clang-query/tool/ClangQuery.cpp

80 ↗

(On Diff #162262)

Maybe make the error message scarier, e.g. "Failed to build AST for some of the files. Results may be incomplete."

82 ↗

(On Diff #162262)

Maybe remove && Status? I don't think it will show up in assertion failure messages anyway.

[george.karpenkov]

@ilya-biryukov Thanks for the review and sorry for the delay :P

clang-tools-extra/clang-query/tool/ClangQuery.cpp

82 ↗

(On Diff #162262)

Oops, you are right.

[ilya-biryukov]

LGTM with a few NITs.
Also consider adding a test for this.

clang-tools-extra/clang-query/tool/ClangQuery.cpp

76 ↗

(On Diff #176440)

These error codes are undocumented, so I would probably stick with a single != 0 check.
But up to you.

77 ↗

(On Diff #176440)

NIT: remove this empty line

127 ↗

(On Diff #176440)

NIT: Maybe make only a single int with a more descriptive name, e.g. ASTStatus.
The return at the end would be return ASTStatus != 0. But up to you.

[george.karpenkov]

clang-tools-extra/clang-query/tool/ClangQuery.cpp

127 ↗

(On Diff #176440)

Then why return ASTStatus != 0 and not just return ASTStatus ? :P

[dblaikie]

clang-tools-extra/trunk/clang-query/tool/ClangQuery.cpp

108

skip 'else after return' ( https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return )

[ilya-biryukov]

clang-tools-extra/clang-query/tool/ClangQuery.cpp

127 ↗

(On Diff #176440)

return ASTStatus is clearly better!

clang-tools-extra/trunk/clang-query/tool/ClangQuery.cpp

if ((!Commands.empty() || !CommandFiles.empty()) && !PreloadFile.empty()) {
     llvm::errs() << argv[0]
                  << ": cannot specify both -c or -f with --preload\n";
     return 1;
   }
 
   ClangTool Tool(OptionsParser.getCompilations(),
                  OptionsParser.getSourcePathList());
   std::vector<std::unique_ptr<ASTUnit>> ASTs;
-  if (Tool.buildASTs(ASTs) != 0)
-    return 1;
+  int Status = Tool.buildASTs(ASTs);
+  int ASTStatus = 0;
+  if (Status == 1) {
+    // Building ASTs failed.
+    return 1;
+  } else if (Status == 2) {
+    ASTStatus |= 1;
+    llvm::errs() << "Failed to build AST for some of the files, "
+                 << "results may be incomplete."
+                 << "\n";
+  } else {
+    assert(Status == 0 && "Unexpected status returned");
+  }
 
   QuerySession QS(ASTs);
 
   if (!Commands.empty()) {
     for (auto I = Commands.begin(), E = Commands.end(); I != E; ++I) {
       QueryRef Q = QueryParser::parse(*I, QS);
       if (!Q->run(llvm::outs(), QS))
         return 1;
while (llvm::Optional<std::string> Line = LE.readLine()) {
       QueryRef Q = QueryParser::parse(*Line, QS);
       Q->run(llvm::outs(), QS);
       llvm::outs().flush();
       if (QS.Terminate)
         break;
     }
   }
 
-  return 0;
+  return ASTStatus;
 }