<div style="font-family: arial, helvetica, sans-serif"><font size="2"><div class="gmail_quote">On Fri, Jun 22, 2012 at 10:41 AM, 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 for the review.<br>
<br>
Who should I CC for the libclang requirements ? Or should I post
those patches on cfe-commits and wait for somebody to chime in :
the subject makes it clear that libclang is somehow affected ?</div></div></blockquote><div><br></div><div>/me didn't notice this is not on cfe-commits :D; yea, post there first.</div><div><br></div><div>Otherwise public IRC is always a good way to grab people's attention. Doug, Argiris or Ted are apparently good approvers according to Chandler ...</div>
<div><br></div><div>Cheers,</div><div>/Manuel</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><div class="im">
<br>
<br>
Cheers,<br>
--<br>
Arnaud de Grandmaison<br>
<br></div><div><div class="h5">
On 06/22/2012 10:28 AM, Manuel Klimek wrote:<br>
</div></div></div>
<blockquote type="cite">
<div style="font-family:arial,helvetica,sans-serif"><font size="2"><div><div class="h5">C/C++ side looks good. Skimmed through the python;
looks good too from my side. You'll probably want an approval
for the location of the code from somebody who understands all
the requirements for libclang (which is not me ;).
<div>
<br>
</div>
<div>Cheers,</div>
</div></div><div>/Manuel<br>
<br>
<div class="gmail_quote"><div><div class="h5">On Fri, Jun 22, 2012 at 1:39 AM,
Arnaud ALLARD DE GRANDMAISON <span dir="ltr"><<a href="mailto:arnaud.allarddegrandmaison@parrot.com" target="_blank">arnaud.allarddegrandmaison@parrot.com</a>></span>
wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">Hi
Manuel and Gregory<br>
<br>
I reworked the C/C++ part of the patch as discussed in
our previous email.<br>
<br>
The main change to the python binding is the handling of
the error code for database loading failure, with the
stuffing of the CompilationDatabaseError exception.
Gregory, you may want to review this part : i have had
troubles to simply raise the CompilationDatabaseError
with the error code (a new argument passed by reference)
in CompilationDatabase.from_result . I have to catch the
exception and re-raise it in
CompilationDatabase.fromDirectory to get access to the
error code.<br>
<div><br>
Cheers,<br>
--<br>
Arnaud de Grandmaison<br>
<br>
</div>
________________________________________<br>
From: Manuel Klimek [<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>]<br>
Sent: Thursday, June 21, 2012 7:37 PM<br>
To: Arnaud ALLARD DE GRANDMAISON<br>
Cc: Gregory Szorc; <a href="mailto:cfe-dev@cs.uiuc.edu" target="_blank">cfe-dev@cs.uiuc.edu</a><br>
Subject: Re: [cfe-dev] CompilationDatabase support added
to libclang and python binding<br>
</div></div><div>
<div><div><div class="h5"><br>
On Thu, Jun 21, 2012 at 7:32 PM, Arnaud de
Grandmaison <<a href="mailto:arnaud.allarddegrandmaison@parrot.com" target="_blank">arnaud.allarddegrandmaison@parrot.com</a><mailto:<a href="mailto:arnaud.allarddegrandmaison@parrot.com" target="_blank">arnaud.allarddegrandmaison@parrot.com</a>>>
wrote:<br>
Thanks Manuel for the review. See my comments below.<br>
<br>
<br>
On 06/21/2012 06:21 PM, Manuel Klimek wrote:<br>
Review of the C/C++ parts:<br>
<br>
General comment: are there different style rules for
libclang? If not, shouldn't function arguments be
FirstLetterCapital?<br>
<br>
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></div></div>
....<div class="im"><br>
CINDEX_LINKAGE CXString
clang_constructUSR_ObjCMethod(const char *name,<br>
unsigned isInstanceMethod,<br>
CXString classUSR);<br></div>
....<div><div class="h5"><br>
<br>
I will fix the patch if there is a preferred style.<br>
<br>
The style rule is<br>
"""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)."""<br>
<br>
Normally the "consistency" rule would win, but if
the code is already inconsistent, I'd go with
sticking to the style guide.<br>
<br>
<br>
<br>
+/**<br>
+ * \brief Find the compile commands used for a
file. The compile commands<br>
+ * must be freed by \c
clang_tooling_CompileCommands_dispose.<br>
+ */<br>
+CINDEX_LINKAGE CXCompileCommands<br>
+clang_tooling_CompilationDatabase_getCompileCommands(CXCompilationDatabase,<br>
+
const char *completeFileName);<br>
<br>
Indent off (here and elsewhere)<br>
<br>
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>
Or break after the opening paren?<br>
clang_tooling_CompilationDatabase_getCompileCommands(<br>
CXCompilationDatabase,<br>
const char *completeFileName);<br>
<br>
<br>
<br>
+ if (strncmp(argv[i],"lookup", 7)==0){<br>
<br>
Is there a case where this is different from using
(or strlen("lookup"))?<br>
I will use strcmp directly.<br>
<br>
<br>
<br>
+// FIXME: do something more usefull with the error
message<br>
+CXCompilationDatabase<br>
+clang_tooling_CompilationDatabase_fromDirectory(const
char *buildDir)<br>
+{<br>
+ std::string errorMsg;<br>
+<br>
+ CompilationDatabase *db =
CompilationDatabase::loadFromDirectory(buildDir,<br>
+
errorMsg);<br>
+<br>
+ if (!db) {<br>
+ fprintf(stderr, "LIBCLANG TOOLING FATAL ERROR:
%s\n", errorMsg.c_str());<br>
+ ::abort();<br>
+ }<br>
+<br>
+ return db;<br>
+}<br>
<br>
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 :)<br>
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>
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.<br>
<br>
<br>
<br>
<br>
+CXCompileCommands<br>
+clang_tooling_CompilationDatabase_getCompileCommands(CXCompilationDatabase
CDb,<br>
+ const char
*completeFileName)<br>
+{<br>
+ if (CompilationDatabase *db =
static_cast<CompilationDatabase *>(CDb)) {<br>
+ const std::vector<CompileCommand><br>
+
CCmd(db->getCompileCommands(completeFileName));<br>
+ if (!CCmd.empty())<br>
+ return new AllocatedCXCompileCommands( CCmd
);<br>
+<br>
<br>
Any reason to not want to have the empty() and
error() cases differentiated?<br>
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>
Fair enough.<br>
<br>
<br>
<br>
+void<br>
+clang_tooling_CompileCommands_dispose(CXCompileCommands
cmds)<br>
+{<br>
+ if (cmds)<br>
+ delete
static_cast<AllocatedCXCompileCommands
*>(cmds);<br>
<br>
isn't the if (cmds) unnecessary?<br>
<br>
Yes. I will fix it.<br>
<br>
Cheers,<br>
/Manuel<br>
<br>
<br>
<br>
Thanks again for the review,<br>
<br>
--<br>
Arnaud de Grandmaison<br>
<br>
<br>
</div></div></div>
</div>
</blockquote>
</div>
<br><span class="HOEnZb"><font color="#888888">
</font></span></div><span class="HOEnZb"><font color="#888888">
</font></span></font></div><span class="HOEnZb"><font color="#888888">
</font></span></blockquote><span class="HOEnZb"><font color="#888888">
<br>
<br>
<pre cols="72">--
Arnaud de Grandmaison
</pre>
</font></span></div>
</blockquote></div><br></font></div>