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

[Manuel Klimek]

On Tue, Jul 10, 2012 at 6:50 PM, Arnaud de Grandmaison
<arnaud.allarddegrandmaison at parrot.com> wrote:
> Patch updated with all your comments.

LGTM.

Cheers,
/Manuel


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