<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">In r247468, thanks for reviewing!</div><br class=""><div><blockquote type="cite" class=""><div class="">On Sep 11, 2015, at 10:24 AM, Manuel Klimek via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" class="">cfe-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">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.</div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Fri, Sep 11, 2015 at 6:32 PM Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com" class="">kyrtzidis@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><blockquote type="cite" class=""><div class="">On Sep 11, 2015, at 12:21 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank" class="">klimek@google.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><div class="gmail_quote"><div dir="ltr" class="">On Thu, Sep 10, 2015 at 8:36 PM Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com" target="_blank" class="">kyrtzidis@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><blockquote type="cite" class=""><div class="">On Sep 10, 2015, at 1:48 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank" class="">klimek@google.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><div class="">@@ -179,11 +185,13 @@ public:</div><div class=""> /// \param Directory The base directory used in the FixedCompilationDatabase.</div><div class=""> static FixedCompilationDatabase *loadFromCommandLine(int &Argc,</div><div class=""> <span class=""> </span>const char *const *Argv,</div><div class="">- Twine Directory = ".");</div><div class="">+ Twine Directory = ".",</div><div class="">+ Twine File = Twine());</div><div class=""> </div><div class="">A fixed compilation database returns the same command lines for all files, thus having a file in the function seems strange.</div></div></div></blockquote><div class=""><br class=""></div></div></div><div style="word-wrap:break-word" class=""><div class=""><div class="">Ah ok, thanks for clarifying.</div></div></div><div style="word-wrap:break-word" class=""><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">What exactly is the use case? So far, the compilation database has been designed for 2 use cases:</div><div class="">1. for a file, get the compile commands; the user already knows the file, no need to get the file</div><div class="">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.</div></div></div></blockquote><div class=""><br class=""></div></div></div><div style="word-wrap:break-word" class=""><div class=""><div class="">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.</div></div></div></blockquote><div class=""><br class=""></div><div class="">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.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><div class="">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.</div><div class=""><br class=""></div><div class="">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 ?</div></div></div></blockquote><div class=""><br class=""></div><div class="">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.</div></div></div></div></blockquote><div class=""><br class=""></div></div></div><div style="word-wrap:break-word" class=""><div class=""><div class="">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.</div></div></div><div style="word-wrap:break-word" class=""><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><div class="gmail_quote"><div class="">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.</div></div></div></div></blockquote><div class=""><br class=""></div></div></div><div style="word-wrap:break-word" class=""><div class=""><div class="">Not sure what <i class="">one</i> 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.</div><div class="">But it is also beyond populating a map, this has an effect on APIs using a CompileCommand. For example:</div><div class=""><br class=""></div><div class=""><span style="white-space:pre-wrap" class=""> </span>void doSomethingWithCompileCommand(const CompileCommand &cmd); </div><div class=""><br class=""></div><div class="">Ah it would be useful to know the file that this command was associated with:</div><div class=""><br class=""></div><div class=""><span style="white-space:pre-wrap" class=""> </span>void doSomethingWithCompileCommand(const CompileCommand &cmd, StringRef filename); </div><div class=""><br class=""></div><div class="">What do I have now ? This is a function taking a command and a string for the filename separately.</div><div class="">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.</div><div class="">This seems like a design smell to me.</div></div></div><div style="word-wrap:break-word" class=""><div class=""><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><div class="gmail_quote"><div class=""><br class=""></div><div class="">Cheers,</div><div class="">/Manuel</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">Thoughts?</div><div class="">/Manuel</div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Wed, Sep 9, 2015 at 9:36 PM Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com" target="_blank" class="">kyrtzidis@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi,<br class=""><br class="">The attached patch exposes the ‘file’ entry in a compilation database command, via the CompileCommand structure.</blockquote></div></div></blockquote></div></div></blockquote></div></div></div></blockquote></div></div></blockquote></div>
_______________________________________________<br class="">cfe-commits mailing list<br class=""><a href="mailto:cfe-commits@lists.llvm.org" class="">cfe-commits@lists.llvm.org</a><br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits<br class=""></div></blockquote></div><br class=""></body></html>