Patch updated with all your comments. Cheers, -- Arnaud de Grandmaison
On 07/10/2012 06:14 PM, Manuel Klimek wrote: > On Tue, Jul 10, 2012 at 5:35 PM, Arnaud A. de Grandmaison > <[email protected]> wrote: >> On 07/10/2012 03:15 PM, Manuel Klimek wrote: >>> On Tue, Jul 10, 2012 at 3:10 PM, Arnaud A. de Grandmaison >>> <[email protected]> wrote: >>>> Is it necessary for 'CompilationDatabase::autoDetectFromSource' to take >>>> a sourcefile to seed the search, instead of using a directory ? >>>> >>>> Using a directory would make this function more generic. >>> I really want this to "just work" for the 99% case. >> That's also what I want :) But I did not express well what I meant then. >> >> What I proposed in my earlier mail, using the original implementation of >> autoDetectFromSource, was to have it check : >> - if the argument is a directory : do not modify it with get_parent. We >> do not want to skip the deepest directory level, >> - otherwise, do a get_parent which acts essentially as 'dirname' (the >> current behaviour) >> >> In the attached patch, I add support for the autodetect feature starting >> from directories. With this implementation, it is made clear whether the >> starting point for the search is a file or a directory. >> >> >>>> For example, in a plugin like clang_complete for vim, to find a >>>> compilation database, you would do something like : >>>> 1 - autoDetectFromSource( basename(sourceFile) ) >>>> 2 - if (no database for sourceFile or sourceFile not found in database) >>>> then autoDetectFromSource( getCwd() ) >>> We can add this behavior to autoDetectFromSource if you think it's >>> important. >>> >>> I exactly *don't* want every tool to have to write their own magic >>> lookup-oh-where's-the-database implementation. I think that'd be hard >>> to support. >> What I meant is that we should provide the low level functionality that >> tools can use to build their own magic upon. The example I gave is >> supposed to take place in the vim plugin, not in clang :). With the >> current implementation of autoDetectFromSource, either the tool author >> has to rewrite its own version for looking into dirs (we do not want >> that), or do something dirty like append a dummy file to the directory >> (we do not want to do that either). >> >> I think we should factorize what can easily be factorized, and not have >> each tool re-implement the low-level logic. High level logic is clearly >> theirs. >> >> >>> Also, this version is the "80% case". We'll add the other 20% incrementally. >>> For example, chandler wants to allow .clangrc files (which we'll >>> probably want when we have clangd anyway) to specify source to build >>> mappings and other stuff. I think we have lots of ways we can improve >>> this, but in general, I want tools to write autoDetectFromSource and >>> be done with it. >> With those 3 functions loadFromDirectory / autoDetectFromSource / >> autoDetectFromDir we cover most --- if not all --- low-level use cases, >> and higher level use can be built above. > Yep, good points, I get it now (code speaks ;) > > In general, LGTM. > > Two comments: > 1. > if (!BuildPath.empty()) { > Compilations.reset(CompilationDatabase::loadFromDirectory(BuildPath, > > ErrorMessage)); > + if (!Compilations) > + Compilations.reset(CompilationDatabase::autoDetectFromDir(BuildPath, > + > ErrorMessage)); > + > > I'd now replace loadFromDirectory with autoDetectFromDir (I'd also > prefer writing Directory, or alternatively rename the other method to > be *Dir, but the inconsistency is definitely worse than all > alternatives) > > 2. > +static CompilationDatabase * > +findCompilationDatabaseFromDirectory(StringRef Directory) { > + CompilationDatabase *DB; > > I'd really prefer keeping the scope small. Is there are reason you > don't stick to the > if (CompilationDatabase *DB = ...) > return DB; > style? > > Cheers, > /Manuel > > -- Arnaud de Grandmaison Senior CPU engineer Business Unit Digital Tuner Parrot S.A. 174, quai de Jemmapes 75010 Paris - France Phone: +33 1 48 03 84 59
>From f4814bd7733553ca0e9dac772c39dca7b645e752 Mon Sep 17 00:00:00 2001 From: "Arnaud A. de Grandmaison" <[email protected]> Date: Tue, 10 Jul 2012 18:47:23 +0200 Subject: [PATCH] Adds support for auto-detection of compilation databases, looking in a directory and all its parents. --- include/clang/Tooling/CompilationDatabase.h | 11 ++++++- lib/Tooling/CompilationDatabase.cpp | 42 +++++++++++++++++++++------ test/Tooling/clang-check-autodetect-dir.cpp | 11 +++++++ tools/clang-check/ClangCheck.cpp | 5 ++- 4 files changed, 56 insertions(+), 13 deletions(-) create mode 100644 test/Tooling/clang-check-autodetect-dir.cpp diff --git a/include/clang/Tooling/CompilationDatabase.h b/include/clang/Tooling/CompilationDatabase.h index 0dec6f8..143c65e 100644 --- a/include/clang/Tooling/CompilationDatabase.h +++ b/include/clang/Tooling/CompilationDatabase.h @@ -83,11 +83,18 @@ public: /// \brief Tries to detect a compilation database location and load it. /// - /// Looks for a compilation database in all parent paths by calling - /// loadFromDirectory. + /// Looks for a compilation database in all parent paths of file 'SourceFile' + /// by calling loadFromDirectory. static CompilationDatabase *autoDetectFromSource(StringRef SourceFile, std::string &ErrorMessage); + /// \brief Tries to detect a compilation database location and load it. + /// + /// Looks for a compilation database in directory 'SourceDir' and all + /// its parent paths by calling loadFromDirectory. + static CompilationDatabase *autoDetectFromDirectory(StringRef SourceDir, + std::string &ErrorMessage); + /// \brief Returns all compile commands in which the specified file was /// compiled. /// diff --git a/lib/Tooling/CompilationDatabase.cpp b/lib/Tooling/CompilationDatabase.cpp index 8e91112..a06343d 100644 --- a/lib/Tooling/CompilationDatabase.cpp +++ b/lib/Tooling/CompilationDatabase.cpp @@ -122,23 +122,47 @@ CompilationDatabase::loadFromDirectory(StringRef BuildDirectory, return Database.take(); } -CompilationDatabase * -CompilationDatabase::autoDetectFromSource(StringRef SourceFile, - std::string &ErrorMessage) { - llvm::SmallString<1024> AbsolutePath(getAbsolutePath(SourceFile)); - StringRef Directory = llvm::sys::path::parent_path(AbsolutePath); +static CompilationDatabase * +findCompilationDatabaseFromDirectory(StringRef Directory) { while (!Directory.empty()) { std::string LoadErrorMessage; - if (CompilationDatabase *DB = loadFromDirectory(Directory, - LoadErrorMessage)) + + if (CompilationDatabase *DB = + CompilationDatabase::loadFromDirectory(Directory, LoadErrorMessage)) return DB; + Directory = llvm::sys::path::parent_path(Directory); } - ErrorMessage = ("Could not auto-detect compilation database for file \"" + - SourceFile + "\"").str(); return NULL; } +CompilationDatabase * +CompilationDatabase::autoDetectFromSource(StringRef SourceFile, + std::string &ErrorMessage) { + llvm::SmallString<1024> AbsolutePath(getAbsolutePath(SourceFile)); + StringRef Directory = llvm::sys::path::parent_path(AbsolutePath); + + CompilationDatabase *DB = findCompilationDatabaseFromDirectory(Directory); + + if (!DB) + ErrorMessage = ("Could not auto-detect compilation database for file \"" + + SourceFile + "\"").str(); + return DB; +} + +CompilationDatabase * +CompilationDatabase::autoDetectFromDirectory(StringRef SourceDir, + std::string &ErrorMessage) { + llvm::SmallString<1024> AbsolutePath(getAbsolutePath(SourceDir)); + + CompilationDatabase *DB = findCompilationDatabaseFromDirectory(AbsolutePath); + + if (!DB) + ErrorMessage = ("Could not auto-detect compilation database from directory \"" + + SourceDir + "\"").str(); + return DB; +} + FixedCompilationDatabase * FixedCompilationDatabase::loadFromCommandLine(int &Argc, const char **Argv, diff --git a/test/Tooling/clang-check-autodetect-dir.cpp b/test/Tooling/clang-check-autodetect-dir.cpp new file mode 100644 index 0000000..2c39504 --- /dev/null +++ b/test/Tooling/clang-check-autodetect-dir.cpp @@ -0,0 +1,11 @@ +// RUN: rm -rf %t +// RUN: mkdir -p %t/abc/def +// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -c %t/test.cpp\",\"file\":\"%t/test.cpp\"}]" | sed -e 's/\\/\\\\/g' > %t/compile_commands.json +// RUN: cp "%s" "%t/test.cpp" +// RUN: clang-check -p "%t/abc/def" "%t/test.cpp" 2>&1|FileCheck %s +// FIXME: Make the above easier. + +// CHECK: C++ requires +invalid; + +// REQUIRES: shell diff --git a/tools/clang-check/ClangCheck.cpp b/tools/clang-check/ClangCheck.cpp index ef4a3ac..d51d90e 100644 --- a/tools/clang-check/ClangCheck.cpp +++ b/tools/clang-check/ClangCheck.cpp @@ -58,8 +58,9 @@ int main(int argc, const char **argv) { if (!Compilations) { std::string ErrorMessage; if (!BuildPath.empty()) { - Compilations.reset(CompilationDatabase::loadFromDirectory(BuildPath, - ErrorMessage)); + Compilations.reset( + CompilationDatabase::autoDetectFromDirectory(BuildPath, ErrorMessage)); + } else { Compilations.reset(CompilationDatabase::autoDetectFromSource( SourcePaths[0], ErrorMessage)); -- 1.7.8.6
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
