<div dir="ltr">Thanks!</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Sep 16, 2015 at 11:30 AM Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com">kyrtzidis@apple.com</a>> wrote:<br></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"><div>In r247832.</div></div><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div>On Sep 12, 2015, at 5:19 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div><br><div><div dir="ltr">Test would be awesome :) Thx!</div><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 11, 2015 at 10:44 PM Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>> wrote:<br></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"><div>In r247468, thanks for reviewing!</div><br><div><blockquote type="cite"></blockquote></div></div><div style="word-wrap:break-word"><div><blockquote type="cite"><div>On Sep 11, 2015, at 10:24 AM, Manuel Klimek via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:</div><br></blockquote></div></div><div style="word-wrap:break-word"><div><blockquote type="cite"><div><div dir="ltr">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><div class="gmail_quote"><div dir="ltr">On Fri, Sep 11, 2015 at 6:32 PM Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>> wrote:<br></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"><div><blockquote type="cite"><div>On Sep 11, 2015, at 12:21 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div><br><div><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"><div class="gmail_quote"><div dir="ltr">On Thu, Sep 10, 2015 at 8:36 PM Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>> wrote:<br></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"><div><blockquote type="cite"><div>On Sep 10, 2015, at 1:48 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div><br><div><div dir="ltr"><div>@@ -179,11 +185,13 @@ public:</div><div> /// \param Directory The base directory used in the FixedCompilationDatabase.</div><div> static FixedCompilationDatabase *loadFromCommandLine(int &Argc,</div><div> <span> </span>const char *const *Argv,</div><div>- Twine Directory = ".");</div><div>+ Twine Directory = ".",</div><div>+ Twine File = Twine());</div><div> </div><div>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><br></div></div></div><div style="word-wrap:break-word"><div><div>Ah ok, thanks for clarifying.</div></div></div><div style="word-wrap:break-word"><div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>What exactly is the use case? So far, the compilation database has been designed for 2 use cases:</div><div>1. for a file, get the compile commands; the user already knows the file, no need to get the file</div><div>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><br></div></div></div><div style="word-wrap:break-word"><div><div>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><br></div><div>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> </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"><div><div>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><br></div><div>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><br></div><div>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><br></div></div></div><div style="word-wrap:break-word"><div><div>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"><div><br><blockquote type="cite"><div><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"><div class="gmail_quote"><div>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><br></div></div></div><div style="word-wrap:break-word"><div><div>Not sure what <i>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>But it is also beyond populating a map, this has an effect on APIs using a CompileCommand. For example:</div><div><br></div><div><span style="white-space:pre-wrap"> </span>void doSomethingWithCompileCommand(const CompileCommand &cmd); </div><div><br></div><div>Ah it would be useful to know the file that this command was associated with:</div><div><br></div><div><span style="white-space:pre-wrap"> </span>void doSomethingWithCompileCommand(const CompileCommand &cmd, StringRef filename); </div><div><br></div><div>What do I have now ? This is a function taking a command and a string for the filename separately.</div><div>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>This seems like a design smell to me.</div></div></div><div style="word-wrap:break-word"><div><div><br></div><blockquote type="cite"><div><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"><div class="gmail_quote"><div><br></div><div>Cheers,</div><div>/Manuel</div><div> </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"><div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>Thoughts?</div><div>/Manuel</div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Sep 9, 2015 at 9:36 PM Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>> wrote:<br></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><br>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></div></blockquote></div></div><div style="word-wrap:break-word"><div><blockquote type="cite"><div>
_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br></div></blockquote></div><br></div></blockquote></div>
</div></blockquote></div><br></div></blockquote></div>