[cfe-commits] [PATCH] Implements support to run standalone tools

Manuel Klimek klimek at google.com
Thu Mar 29 10:51:30 PDT 2012


On Thu, Mar 29, 2012 at 7:41 PM, Marshall Clow <mclow.lists at gmail.com> wrote:
> On Mar 29, 2012, at 10:21 AM, Manuel Klimek wrote:
>
>> On Mar 29, 2012 5:25 PM, "Marshall Clow" <mclow.lists at gmail.com> 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:mclow.lists at gmail.com>
>
> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait).
>        -- Yu Suzuki
>




More information about the cfe-commits mailing list