<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Thanks Manuel for the review. See my
comments below.<br>
<br>
On 06/21/2012 06:21 PM, Manuel Klimek wrote:<br>
</div>
<blockquote
cite="mid:CAOsfVvmZ_Bjoy3y8JVC4rzUb21m-831fmqojVU5CQw841Bm8Zw@mail.gmail.com"
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>
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.<br>
<br>
<blockquote
cite="mid:CAOsfVvmZ_Bjoy3y8JVC4rzUb21m-831fmqojVU5CQw841Bm8Zw@mail.gmail.com"
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>
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.<br>
<br>
<blockquote
cite="mid:CAOsfVvmZ_Bjoy3y8JVC4rzUb21m-831fmqojVU5CQw841Bm8Zw@mail.gmail.com"
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>
I will use strcmp directly.<br>
<br>
<blockquote
cite="mid:CAOsfVvmZ_Bjoy3y8JVC4rzUb21m-831fmqojVU5CQw841Bm8Zw@mail.gmail.com"
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>
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) ?<br>
<br>
<blockquote
cite="mid:CAOsfVvmZ_Bjoy3y8JVC4rzUb21m-831fmqojVU5CQw841Bm8Zw@mail.gmail.com"
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>
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.<br>
<br>
<blockquote
cite="mid:CAOsfVvmZ_Bjoy3y8JVC4rzUb21m-831fmqojVU5CQw841Bm8Zw@mail.gmail.com"
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>
Yes. I will fix it.<br>
<br>
<blockquote
cite="mid:CAOsfVvmZ_Bjoy3y8JVC4rzUb21m-831fmqojVU5CQw841Bm8Zw@mail.gmail.com"
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,<br>
<pre class="moz-signature" cols="72">--
Arnaud de Grandmaison
</pre>
</body>
</html>