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