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 > <[email protected]> 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 >>> <[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 >> >
-- 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
