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. 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. Cheers, /Manuel > > -- 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
