<div style="font-family: arial, helvetica, sans-serif"><font size="2"><div class="gmail_quote">On Thu, Jun 21, 2012 at 7:32 PM, Arnaud de Grandmaison <span dir="ltr"><<a href="mailto:arnaud.allarddegrandmaison@parrot.com" target="_blank">arnaud.allarddegrandmaison@parrot.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>Thanks Manuel for the review. See my
comments below.<div class="im"><br>
<br>
On 06/21/2012 06:21 PM, Manuel Klimek wrote:<br>
</div></div><div class="im">
<blockquote type="cite">
<div style="font-family:arial,helvetica,sans-serif"><font size="2">
<div>Review of the C/C++ parts:</div>
<div><br>
</div>
<div>General comment: are there different style rules for
libclang? If not, shouldn't function arguments be
FirstLetterCapital?</div>
<div><br>
</div>
</font></div>
</blockquote></div>
Looking at CIndex.h, we can see a mix of different styles
(underscores, camelCase, CamelCase).<br>
Example : <br>
CINDEX_LINKAGE CXTranslationUnit clang_parseTranslationUnit(CXIndex
CIdx,<br>
const char
*source_filename<br>
...<br>
CINDEX_LINKAGE CXString clang_constructUSR_ObjCMethod(const char
*name,<br>
unsigned
isInstanceMethod,<br>
CXString
classUSR);<br>
...<br>
<br>
I will fix the patch if there is a preferred style.</div></blockquote><div><br></div><div>The style rule is</div>"""Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats)."""</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">Normally the "consistency" rule would win, but if the code is already inconsistent, I'd go with sticking to the style guide.</div><div class="gmail_quote">
<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div class="im"><br>
<br>
<blockquote type="cite">
<div style="font-family:arial,helvetica,sans-serif"><font size="2">
<div>+/**</div>
<div>+ * \brief Find the compile commands used for a file. The
compile commands </div>
<div>+ * must be freed by \c
clang_tooling_CompileCommands_dispose.</div>
<div>+ */</div>
<div>+CINDEX_LINKAGE CXCompileCommands</div>
<div>+clang_tooling_CompilationDatabase_getCompileCommands(CXCompilationDatabase,</div>
<div>+ const
char *completeFileName);</div>
<div><br>
</div>
<div>Indent off (here and elsewhere)</div>
<div>
<br>
</div>
</font></div>
</blockquote></div>
By indenting those cases, it will exceed the 80 cols. For sure I can
use shorter parameter names, even though the name in the prototype
should be self descriptive --- or simply add the description in the
comment.</div></blockquote><div><br></div><div>Or break after the opening paren?</div><div><span style="font-family:arial,helvetica,sans-serif">clang_tooling_</span><span style="font-family:arial,helvetica,sans-serif">CompilationDatabase_</span><span style="font-family:arial,helvetica,sans-serif">getCompileCommands(</span></div>
<div><span style="font-family:arial,helvetica,sans-serif"> CXCompilationDatabase,</span></div><div><span style="font-family:arial,helvetica,sans-serif"> const char *completeFileName);</span></div><div><div style="font-family:arial,helvetica,sans-serif">
<font size="2"><div><br></div></font></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div class="im"><br>
<br>
<blockquote type="cite">
<div style="font-family:arial,helvetica,sans-serif"><font size="2">
<div>
<div>+ if (strncmp(argv[i],"lookup", 7)==0){</div>
</div>
<div><br>
</div>
<div>Is there a case where this is different from using (or
strlen("lookup"))?</div>
</font></div>
</blockquote></div>
I will use strcmp directly.<div class="im"><br>
<br>
<blockquote type="cite">
<div style="font-family:arial,helvetica,sans-serif"><font size="2">
<div><br>
</div>
<div>+// FIXME: do something more usefull with the error
message</div>
<div>+CXCompilationDatabase</div>
<div>+clang_tooling_CompilationDatabase_fromDirectory(const
char *buildDir)</div>
<div>+{</div>
<div>+ std::string errorMsg;</div>
<div>+</div>
<div>+ CompilationDatabase *db =
CompilationDatabase::loadFromDirectory(buildDir,</div>
<div>+
errorMsg);</div>
<div>+</div>
<div>+ if (!db) {</div>
<div>+ fprintf(stderr, "LIBCLANG TOOLING FATAL ERROR:
%s\n", errorMsg.c_str());</div>
<div>+ ::abort();</div>
<div>+ }</div>
<div>+</div>
<div>+ return db;</div>
<div>+}</div>
<div><br>
</div>
<div>With "more useful" you hopefully mean to expose it into
to the caller? Because this seems to be something the user
of a tool might control (for example, via configuration), so
aborting seems impolite :)</div>
</font></div>
</blockquote></div>
Yep. Abort will go away. The caller needs to be able to retry.<br>
What to do with the error message. Is printing it to stderr ok ? Or
shouldn't we instead return an error code to the user, as there are
several possible failures (from the buildDir does not exists, to no
database found, or loading error) ?</div></blockquote><div><br></div><div>Yep, error code to the user - this is going to be used from python code, and people will want to react to configuration errors / user input differently. </div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div class="im"><br>
<br>
<blockquote type="cite">
<div style="font-family:arial,helvetica,sans-serif"><font size="2">
<div><br>
</div>
<div>
<div>+CXCompileCommands</div>
<div>+clang_tooling_CompilationDatabase_getCompileCommands(CXCompilationDatabase
CDb,</div>
<div>+ const char
*completeFileName)</div>
<div>+{</div>
<div>+ if (CompilationDatabase *db =
static_cast<CompilationDatabase *>(CDb)) {</div>
<div>+ const std::vector<CompileCommand></div>
<div>+
CCmd(db->getCompileCommands(completeFileName));</div>
<div>
+ if (!CCmd.empty())</div>
<div>+ return new AllocatedCXCompileCommands( CCmd );</div>
<div>+</div>
</div>
<div><br>
</div>
<div>Any reason to not want to have the empty() and error()
cases differentiated?</div>
</font></div>
</blockquote></div>
I wanted to avoid allocation + garbage collection for essentially
empty data. Now, it is true that the user can not differentiate
between empty() and error. However, for this function, I do not see
how the user could use this difference --- should he be able to.</div></blockquote><div><br></div><div>Fair enough.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div class="im"><br>
<br>
<blockquote type="cite">
<div style="font-family:arial,helvetica,sans-serif"><font size="2">
<div>
<div>+void</div>
<div>
+clang_tooling_CompileCommands_dispose(CXCompileCommands
cmds)</div>
<div>+{</div>
<div>+ if (cmds)</div>
<div>+ delete static_cast<AllocatedCXCompileCommands
*>(cmds);</div>
</div>
<div><br>
</div>
<div>isn't the if (cmds) unnecessary?</div>
<div><br>
</div>
</font></div>
</blockquote></div>
Yes. I will fix it.<br>
<br>
<blockquote type="cite">
<div style="font-family:arial,helvetica,sans-serif"><font size="2">
<div>Cheers,</div>
<div>/Manuel</div>
<div><br>
</div>
</font><br>
</div>
</blockquote>
<br>
Thanks again for the review,<span class="HOEnZb"><font color="#888888"><br>
<pre cols="72">--
Arnaud de Grandmaison
</pre>
</font></span></div>
</blockquote></div><br></font></div>