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 <kyrtzi...@apple.com> wrote: > On Sep 11, 2015, at 12:21 AM, Manuel Klimek <kli...@google.com> wrote: > > On Thu, Sep 10, 2015 at 8:36 PM Argyrios Kyrtzidis <kyrtzi...@apple.com> > wrote: > >> On Sep 10, 2015, at 1:48 AM, Manuel Klimek <kli...@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 <kyrtzi...@apple.com> >> wrote: >> >>> Hi, >>> >>> The attached patch exposes the ‘file’ entry in a compilation database >>> command, via the CompileCommand structure. >> >>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
