<div style="font-family: arial, helvetica, sans-serif"><font size="2">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>/Manuel<br><br><div class="gmail_quote">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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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><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>
...<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>
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></blockquote></div><br>
</div></font></div>