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

[Arnaud de Grandmaison]

Committed as r159998.

On 07/10/2012 06:55 PM, Manuel Klimek wrote:
> 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
>>
>


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