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

[Manuel Klimek]

On Mon, May 7, 2012 at 8:34 AM, Chandler Carruth <chandlerc at google.com> wrote:
> 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

Fixed the quoting, as requested on IRC.

> +// 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...

I think they're relevant for the same reasons - unless I can check the
tests on windows I'd rather be safe here.

> 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.

Those changes were auto-generated by svn - I didn't mess with them.

> 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.

Since you didn't reply on my "say it one more time" I'm leaving it as
is unless you tell me once and for all to remove it ;)

> 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.

As discussed in IRC, according to Bigcheese this will still work on
Windows. We'll want a more thorough fix in the future.

> With this fixes, LGTM.

Michael lgtm'ed on IRC, so I submitted as r156299. Let me know if you
disagree with my assessment and I'll fix it up.

Thanks,
/Manuel