[cfe-commits] PATCH: Rough-cut auto-detection of a build directory location for the tooling framework

[Manuel Klimek]

On Tue, Jul 10, 2012 at 5:35 PM, Arnaud A. de Grandmaison
<arnaud.allarddegrandmaison at parrot.com> wrote:
> On 07/10/2012 03:15 PM, Manuel Klimek wrote:
>> On Tue, Jul 10, 2012 at 3:10 PM, Arnaud A. de Grandmaison
>> <arnaud.allarddegrandmaison at parrot.com> 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