In r247468, thanks for reviewing! > On Sep 11, 2015, at 10:24 AM, Manuel Klimek via cfe-commits > <cfe-commits@lists.llvm.org> wrote: > > 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 > <mailto:kyrtzi...@apple.com>> wrote: >> On Sep 11, 2015, at 12:21 AM, Manuel Klimek <kli...@google.com >> <mailto:kli...@google.com>> wrote: >> >> On Thu, Sep 10, 2015 at 8:36 PM Argyrios Kyrtzidis <kyrtzi...@apple.com >> <mailto:kyrtzi...@apple.com>> wrote: >>> On Sep 10, 2015, at 1:48 AM, Manuel Klimek <kli...@google.com >>> <mailto: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 >>> <mailto: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
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
