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

[Marshall Clow]

On Mar 29, 2012, at 10:51 AM, Manuel Klimek wrote:

> 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.

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: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