[cfe-commits] [PATCH] Make ClangTool chdir into the compilation directory

[Chandler Carruth]

Comments on the patch:

Index: test/Tooling/clang-check-chdir.cpp
===================================================================
--- test/Tooling/clang-check-chdir.cpp (revision 0)
+++ test/Tooling/clang-check-chdir.cpp (revision 0)
@@ -0,0 +1,18 @@
+// Verifies that paths are resolved relatively to the directory specified
in the
+// compilation database.
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo '[{"directory":"%t","command":"clang -c test.cpp
-I.","file":"%t/test.cpp"}]' > %t/compile_commands.json
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: touch "%t/clang-check-test.h"
+// RUN: clang-check "%t" "%t/test.cpp" 2>&1|FileCheck %s
+// FIXME: Make the above easier.
+
+#include "clang-check-test.h"
+
+// CHECK: C++ requires
+invalid;
+
+// FIXME: JSON doesn't like path separator '\', on Win32 hosts.
+// FIXME: clang-check doesn't like gcc driver on cygming.
+// XFAIL: cygwin,mingw32,win32

These FIXMEs and the XFAIL look like copy-paste from other tests, and not
relevant to this test...

Also, the patch mentions property changes on this file -- please don't
explicitly set properties on text files, subversion clients should
auto-detect the correct eol-style to use.

Index: include/clang/Tooling/Tooling.h
===================================================================
--- include/clang/Tooling/Tooling.h (revision 156059)
+++ include/clang/Tooling/Tooling.h (working copy)
@@ -35,6 +35,7 @@
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Driver/Util.h"
+#include "clang/Tooling/CompilationDatabase.h"
 #include <string>
 #include <vector>

@@ -50,8 +51,6 @@

 namespace tooling {

-class CompilationDatabase;
-
 /// \brief Interface to generate clang::FrontendActions.
 class FrontendActionFactory {
 public:
@@ -169,9 +168,8 @@
   FileManager &getFiles() { return Files; }

  private:
-  // We store command lines as pair (file name, command line).
-  typedef std::pair< std::string, std::vector<std::string> > CommandLine;
-  std::vector<CommandLine> CommandLines;
+  // We store compile commands as pair (file name, compile command).
+  std::vector< std::pair<std::string, CompileCommand> > CompileCommands;

Don't add spaces after the "<" unnecessarily.

Index: lib/Tooling/Tooling.cpp
===================================================================
--- lib/Tooling/Tooling.cpp (revision 156059)
+++ lib/Tooling/Tooling.cpp (working copy)
@@ -286,9 +280,20 @@

 int ClangTool::run(FrontendActionFactory *ActionFactory) {
   bool ProcessingFailed = false;
-  for (unsigned I = 0; I < CommandLines.size(); ++I) {
-    std::string File = CommandLines[I].first;
-    std::vector<std::string> &CommandLine = CommandLines[I].second;
+  for (unsigned I = 0; I < CompileCommands.size(); ++I) {
+    std::string File = CompileCommands[I].first;
+    // 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
switched
+    // during runtime of the tool. Fixing this depends on having a file
system
+    // abstraction that allows openat() style interactions.
+    if (chdir(CompileCommands[I].second.Directory.c_str()))

This is also platform-specific. I think it's OK because it's a hack, and a
temporary one that we don't want to enshrine in an API that gets different
platform specific implementations, but you should mention it, and keep the
XFAILs in the tests are necessary.


With this fixes, LGTM.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120506/73806aa6/attachment.html>