[Tooling] Returns non-zero status code when files are skipped.

[hokein]

include/clang/Tooling/Tooling.h

331 ↗

(On Diff #130840)

I'm not sure whether this new behavior will cause any regression issues (many tools invoke this method like if (Tool.run(...)))

• before the patch, return 0 if some files are skipped (we think it succeeds)
• after the patch, return 2 if some files are skipped (we think it fails)

Maybe @bkramer has more words on it?

[ioeric]

include/clang/Tooling/Tooling.h

331 ↗

(On Diff #130840)

I should have mentioned in the path summary that I think this would not regress tools. I checked call sites of this API. I think it's reasonable for most tools to treat skipping files as failure (if they care about failure; some tools simply ignore the status code). Tools that care about this are already ignoring the failures (e.g. https://github.com/llvm-mirror/clang/blob/master/tools/clang-refactor/ClangRefactor.cpp#L630). But there might be something that I am missing. I would certainly like more opinions on this.

[bkramer]

lib/Tooling/Tooling.cpp

404 ↗

(On Diff #130840)

This comment explains why the implementation doesn't error. Can you make sure the xargs use case is still working properly?

[ioeric]

lib/Tooling/Tooling.cpp

404 ↗

(On Diff #130840)

I somehow missed the big FIXME... thanks for the catch!

I don't think this is a very typical use case that should affect design decision here, and I would expect xargs users to do something like xargs tool $@ || true if they really want to ignore errors. WDYT?

[bkramer]

lib/Tooling/Tooling.cpp

404 ↗

(On Diff #130840)

Yeah. I think the only important thing is that xargs doesn't stop after the first error. But that seems to be the default behavior of xargs?

[ioeric]

lib/Tooling/Tooling.cpp

404 ↗

(On Diff #130840)

Looks like this is the case, from the manual:

If  any  invocation of the command exits with a status of 255, xargs will stop immediately without reading any further input.  An error message is issued on stderr when this happens.

Should I just remove the FIXME?

[ioeric]

Ping?

[bkramer]

Removing FIXME seems right to me.

[ioeric]

removed FIXME

cfe/trunk/include/clang/Tooling/Tooling.h

public:
   void appendArgumentsAdjuster(ArgumentsAdjuster Adjuster);
 
   /// \brief Clear the command line arguments adjuster chain.
   void clearArgumentsAdjusters();
 
   /// Runs an action over all files specified in the command line.
   ///
   /// \param Action Tool action.
+  ///
+  /// \returns 0 on success; 1 if any error occured; 2 if there is no error but
+  /// some files are skipped due to missing compile commands.
   int run(ToolAction *Action);
 
   /// \brief Create an AST for each file specified in the command line and
   /// append them to ASTs.
   int buildASTs(std::vector<std::unique_ptr<ASTUnit>> &ASTs);
 
   /// \brief Returns the file manager used in the tool.
   ///

cfe/trunk/lib/Tooling/Tooling.cpp

int ClangTool::run(ToolAction *Action) {
   if (SeenWorkingDirectories.insert("/").second)
     for (const auto &MappedFile : MappedFileContents)
       if (llvm::sys::path::is_absolute(MappedFile.first))
         InMemoryFileSystem->addFile(
             MappedFile.first, 0,
             llvm::MemoryBuffer::getMemBuffer(MappedFile.second));
 
   bool ProcessingFailed = false;
+  bool FileSkipped = false;
   for (const auto &SourcePath : SourcePaths) {
     std::string File(getAbsolutePath(SourcePath));
 
     // Currently implementations of CompilationDatabase::getCompileCommands can
     // change the state of the file system (e.g.  prepare generated headers), so
     // this method needs to run right before we invoke the tool, as the next
     // file may require a different (incompatible) state of the file system.
     //
     // FIXME: Make the compilation database interface more explicit about the
     // requirements to the order of invocation of its members.
     std::vector<CompileCommand> CompileCommandsForFile =
         Compilations.getCompileCommands(File);
     if (CompileCommandsForFile.empty()) {
-      // FIXME: There are two use cases here: doing a fuzzy
-      // "find . -name '*.cc' |xargs tool" match, where as a user I don't care
-      // about the .cc files that were not found, and the use case where I
-      // specify all files I want to run over explicitly, where this should
-      // be an error. We'll want to add an option for this.
       llvm::errs() << "Skipping " << File << ". Compile command not found.\n";
+      FileSkipped = true;
       continue;
     }
     for (CompileCommand &CompileCommand : CompileCommandsForFile) {
       // FIXME: chdir is thread hostile; on the other hand, creating the same
       // behavior as chdir is complex: chdir resolves the path once, thus
       // guaranteeing that all subsequent relative path operations work
       // on the same path the original chdir resulted in. This makes a
       // difference for example on network filesystems, where symlinks might be
for (CompileCommand &CompileCommand : CompileCommandsForFile) {
       }
       // Return to the initial directory to correctly resolve next file by
       // relative path.
       if (OverlayFileSystem->setCurrentWorkingDirectory(InitialDirectory.c_str()))
         llvm::report_fatal_error("Cannot chdir into \"" +
                                  Twine(InitialDirectory) + "\n!");
     }
   }
-  return ProcessingFailed ? 1 : 0;
+  return ProcessingFailed ? 1 : (FileSkipped ? 2 : 0);
 }
 
 namespace {
 
 class ASTBuilderAction : public ToolAction {
   std::vector<std::unique_ptr<ASTUnit>> &ASTs;
 
 public: