Comments on the patch:<div><br></div><div><div>Index: test/Tooling/clang-check-chdir.cpp</div><div>===================================================================</div><div>--- test/Tooling/clang-check-chdir.cpp<span class="Apple-tab-span" style="white-space:pre">    </span>(revision 0)</div>
<div>+++ test/Tooling/clang-check-chdir.cpp<span class="Apple-tab-span" style="white-space:pre">        </span>(revision 0)</div><div>@@ -0,0 +1,18 @@</div><div>+// Verifies that paths are resolved relatively to the directory specified in the</div>
<div>+// compilation database.</div><div>+// RUN: rm -rf %t</div><div>+// RUN: mkdir %t</div><div>+// RUN: echo '[{"directory":"%t","command":"clang -c test.cpp -I.","file":"%t/test.cpp"}]' > %t/compile_commands.json</div>
<div>+// RUN: cp "%s" "%t/test.cpp"</div><div>+// RUN: touch "%t/clang-check-test.h"</div><div>+// RUN: clang-check "%t" "%t/test.cpp" 2>&1|FileCheck %s</div><div>+// FIXME: Make the above easier.</div>
<div>+</div><div>+#include "clang-check-test.h"</div><div>+</div><div>+// CHECK: C++ requires</div><div>+invalid;</div><div>+</div><div>+// FIXME: JSON doesn't like path separator '\', on Win32 hosts.</div>
<div>+// FIXME: clang-check doesn't like gcc driver on cygming.</div><div>+// XFAIL: cygwin,mingw32,win32</div><div><br></div><div>These FIXMEs and the XFAIL look like copy-paste from other tests, and not relevant to this test...</div>
<div><br></div><div>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.</div><div><br></div>
<div>Index: include/clang/Tooling/Tooling.h</div><div>===================================================================</div><div>--- include/clang/Tooling/Tooling.h<span class="Apple-tab-span" style="white-space:pre">    </span>(revision 156059)</div>
<div>+++ include/clang/Tooling/Tooling.h<span class="Apple-tab-span" style="white-space:pre">   </span>(working copy)</div><div>@@ -35,6 +35,7 @@</div><div> #include "clang/Basic/FileManager.h"</div><div> #include "clang/Basic/LLVM.h"</div>
<div> #include "clang/Driver/Util.h"</div><div>+#include "clang/Tooling/CompilationDatabase.h"</div><div> #include <string></div><div> #include <vector></div><div> </div><div>@@ -50,8 +51,6 @@</div>
<div> </div><div> namespace tooling {</div><div> </div><div>-class CompilationDatabase;</div><div>-</div><div> /// \brief Interface to generate clang::FrontendActions.</div><div> class FrontendActionFactory {</div><div> public:</div>
<div>@@ -169,9 +168,8 @@</div><div>   FileManager &getFiles() { return Files; }</div><div> </div><div>  private:</div><div>-  // We store command lines as pair (file name, command line).</div><div>-  typedef std::pair< std::string, std::vector<std::string> > CommandLine;</div>
<div>-  std::vector<CommandLine> CommandLines;</div><div>+  // We store compile commands as pair (file name, compile command).</div><div>+  std::vector< std::pair<std::string, CompileCommand> > CompileCommands;</div>
<div><br></div><div>Don't add spaces after the "<" unnecessarily.</div><div><br></div><div>Index: lib/Tooling/Tooling.cpp</div><div>===================================================================</div>
<div>--- lib/Tooling/Tooling.cpp<span class="Apple-tab-span" style="white-space:pre">   </span>(revision 156059)</div><div>+++ lib/Tooling/Tooling.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div>
<div>@@ -286,9 +280,20 @@</div><div> </div><div> int ClangTool::run(FrontendActionFactory *ActionFactory) {</div><div>   bool ProcessingFailed = false;</div><div>-  for (unsigned I = 0; I < CommandLines.size(); ++I) {</div>
<div>-    std::string File = CommandLines[I].first;</div><div>-    std::vector<std::string> &CommandLine = CommandLines[I].second;</div><div>+  for (unsigned I = 0; I < CompileCommands.size(); ++I) {</div><div>
+    std::string File = CompileCommands[I].first;</div><div>+    // FIXME: chdir is thread hostile; on the other hand, creating the same</div><div>+    // behavior as chdir is complex: chdir resolves the path once, thus</div>
<div>+    // guaranteeing that all subsequent relative path operations work</div><div>+    // on the same path the original chdir resulted in. This makes a difference</div><div>+    // for example on network filesystems, where symlinks might be switched </div>
<div>+    // during runtime of the tool. Fixing this depends on having a file system</div><div>+    // abstraction that allows openat() style interactions.</div><div>+    if (chdir(CompileCommands[I].second.Directory.c_str()))</div>
<div><br></div><div>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.</div>
</div><div><br></div><div><br></div><div>With this fixes, LGTM.</div>