PATCH: Expose the 'file' that is associated with a compile database command

[Manuel Klimek via cfe-commits]

Ok, looked at the original patch again, and if we're fixing the
FixedCompilationDatabase to only insert the file when it actually produces
a CompileCommand it seems to be fine.

On Fri, Sep 11, 2015 at 6:32 PM Argyrios Kyrtzidis <kyrtzidis at apple.com>
wrote:

> On Sep 11, 2015, at 12:21 AM, Manuel Klimek <klimek at google.com> wrote:
>
> On Thu, Sep 10, 2015 at 8:36 PM Argyrios Kyrtzidis <kyrtzidis at apple.com>
> wrote:
>
>> On Sep 10, 2015, at 1:48 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>> @@ -179,11 +185,13 @@ public:
>>    /// \param Directory The base directory used in the
>> FixedCompilationDatabase.
>>    static FixedCompilationDatabase *loadFromCommandLine(int &Argc,
>>                                                         const char
>> *const *Argv,
>> -                                                       Twine Directory =
>> ".");
>> +                                                       Twine Directory =
>> ".",
>> +                                                       Twine File =
>> Twine());
>>
>> A fixed compilation database returns the same command lines for all
>> files, thus having a file in the function seems strange.
>>
>>
>> Ah ok, thanks for clarifying.
>>
>>
>> What exactly is the use case? So far, the compilation database has been
>> designed for 2 use cases:
>> 1. for a file, get the compile commands; the user already knows the file,
>> no need to get the file
>> 2. get all compile commands; for that, we have the getAllFiles() method,
>> so a user can get all known files (for compilation databases that support
>> that), and then get the compile command line.
>>
>>
>> It’s #2, I want to get all compile commands. But it seems really strange
>> to me that the ‘file’ starts as a property of the compile command in the
>> json file but then it gets dropped and I need to do work to re-associate
>> the files with the compile commands again.
>>
>
> The JSON file format is one possible implementation for the
> compilation-database interface. The FixedCompilationDatabase is another
> one, that doesn't have any information on files.
>
>
>> I need to get a list of all the files and then for each one do a lookup
>> to get the associated commands. I then have to maintain this association
>> myself, passing a command along with its file separately or the structure
>> that keeps track of the association.
>>
>> It seems simpler to me to include the file that was associated with the
>> command (if the compilation database supports that) along with the command,
>> is there a downside I’m missing ?
>>
>
> Well, to me, it's a design question - if it also makes sense to have a
> CompileCommand without a file associated with it, putting the file in
> there, while convenient, is a design smell.
>
>
> It can be optional to communicate that it may not be there. Note that,
> IMO, having multiple files and compile commands for them is the
> overwhelmingly most common use of the compilation database.
>
> That said, I'm happy to be convinced that I'm wrong :) I guess I don't see
> yet that keeping track of the files outside is more than one line of extra
> code.
>
>
> Not sure what *one* line this is, I have to declare a map and then
> populate it, no ? And to do it in c-index-test it would take way more that
> one line.
> But it is also beyond populating a map, this has an effect on APIs using a
> CompileCommand. For example:
>
> void doSomethingWithCompileCommand(const CompileCommand &cmd);
>
> Ah it would be useful to know the file that this command was associated
> with:
>
> void doSomethingWithCompileCommand(const CompileCommand &cmd, StringRef
> filename);
>
> What do I have now ? This is a function taking a command and a string for
> the filename separately.
> Is this flexibility useful ? Does it make sense to pass any filename there
> ? No there’s only one filename that the command was associated with so this
> ‘flexibility’ only increases the complexity of using the function. And this
> can propagate to other function’s callees.
> This seems like a design smell to me.
>
>
> Cheers,
> /Manuel
>
>
>>
>>
>> Thoughts?
>> /Manuel
>>
>> On Wed, Sep 9, 2015 at 9:36 PM Argyrios Kyrtzidis <kyrtzidis at apple.com>
>> wrote:
>>
>>> Hi,
>>>
>>> The attached patch exposes the ‘file’ entry in a compilation database
>>> command, via the CompileCommand structure.
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150911/c7cceb53/attachment-0001.html>