On Mar 29, 2012, at 10:51 AM, Manuel Klimek wrote: > On Thu, Mar 29, 2012 at 7:41 PM, Marshall Clow <[email protected]> wrote: >> On Mar 29, 2012, at 10:21 AM, Manuel Klimek wrote: >> >>> On Mar 29, 2012 5:25 PM, "Marshall Clow" <[email protected]> wrote: >>>> >>>> On Mar 29, 2012, at 4:49 AM, Manuel Klimek wrote: >>>> >>>>> Updated patch: now injecting the CompilationDatabase into the >>>>> ClangTool, plus some clean ups. >>>>> >>>>> Please let me know >>>>> - if that's in line with what you thought >>>> >>>> Yes, this is more in line with what I was thinking. >>>> >>>> But… ;-) >>>> >>>> I think that: >>>> >>>> 1) >>>> + /// FIXME: Currently only supports JSON compilation databases, which >>>> + /// are named 'compile_commands.json' in the given directory. Extend >>>> this >>>> + /// for other build types (like ninja build files). >>>> + static CompilationDatabase *loadFromDirectory(StringRef BuildDirectory, >>>> + std::string >>>> &ErrorMessage); >>>> >>>> doesn't belong in CompilationDatabase. >>>> The reason is that in my suggested usage, there will not be a "build >>>> directory" to load the compilation database from. >>>> I agree that JSONCompilationDatabase and "MakefileCompilationDatabase" (or >>>> whatever) need this, but for simple tools, I'd like to be able to specify >>>> all of the options on the command line. >>>> Things like: >>>> find . -name \*.cpp -exec tool -option1 -option2 -option3 {} \; >>> >>> I'm not clear whether you're saying the design I proposed a few mails >>> ago is not a good goal (which is pretty much what I now implemented, >>> re-pasting to make sure we're talking about the same reference): >>> <define tool command lines> >>> int main(...) { >>> // Make argc and argv references so we can change them? Kind of >>> yuck, alternative ideas welcome. >>> llvm::OwningPtr<CompilationDatabase> >>> Database(createSimpleCompilationDatabase(argc, argv)); >>> <do llvm command line parsing on rest of argc, argv> >>> if (!Database) { >>> Database.reset(loadCompilationDatabase(BuildDirectory)); >>> } >>> ClangTool Tool(Database.get(), Filenames); >>> } >> >> Nope. Not saying that. >> I'm happy with this. >> >> >>> ... or whether you're saying that I should implement the >>> "createSimpleCompilationDatabase(argc, argv)" right away? (which I'm >>> fine with, but I don't want to bloat this patch with more features) >> >> Nope. Not saying that either. >> You (or I) can write createSimpleCompilationDatabase(argc, argv) later. >> >>> The reason I think the creation of the SimpleCompilationDatabase (I'm >>> not happy with the name yet) should be different from the >>> CompilationDatabase::loadFromDirectory(...) is that I want to keep the >>> normal llvm command line parsing outside of the CompilationDatabase, >>> as otherwise it gets hard to add extra parameters when writing a tool, >>> which is pretty common. >> >> That's fine with me, too. >> >> My point was that given: >> 1) that we're going to have a SimpleCompilationDatabase (possibly with a >> different name) sometime down the road, and >> 2) that "loadFromDirectory" doesn't really make sense when you're getting >> everything from the command line, >> >> then, does loadFromDirectory really belong in the base class? >> [ because then SimpleCompilationDatabase will have to implement it - even if >> it just returns an error ] > > Ah, but loadFromDirectory is just a static factory method - none of > the children have to implement this.
Ah - missed that somehow. Sorry. > If this is not a common enough pattern in llvm, I'm happy to pull the > method out. > The reasoning I had to put it in is that it's basically the > "canonical" factory for CompilationDatabases and that when somebody > implements a new CompilationDatabase subclass, I want them to consider > adding a detection into this factory method (so coupling is strong). > Also, I think SimpleCompilationDatabase is the odd special case - I > expect the majority of CompilationDatabases to come from a build > system. > >> It's an interface question, more than an architecture question. >> >> Hopefully this is clearer. > > Yep, thanks for clarifying. And thanks for the discussion, I'm really > happy with the path this is taking design-wise. Me, too. Thanks! -- Marshall Marshall Clow Idio Software <mailto:[email protected]> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait). -- Yu Suzuki _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
