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

[Marshall Clow]

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 ]

It's an interface question, more than an architecture question.

Hopefully this is clearer.

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