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

[Arnaud de Grandmaison]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Adds-support-for-auto-detection-of-compilation-datab.patch
Type: text/x-patch
Size: 5642 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120710/2fb9d652/attachment.bin>