<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><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><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><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>
<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><div><br></div><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><div>Cheers,</div><div>/Manuel</div><div><br></div><div class="gmail_quote">
On Tue, Jun 19, 2012 at 10:19 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">
Hi Manuel & Gregory,<br>
<br>
Here are the updated patches.<br>
<br>
The only significant change is in the python binding, where a<br>
CompilationCommand holds a reference to the associated<br>
CompilationCommands, preventing garbage collection. This is now also tested.<br>
<div><br>
Cheers,<br>
--<br>
Arnaud de Grandmaison<br>
<br>
</div><div><div>On 06/18/2012 09:52 AM, Arnaud de Grandmaison wrote:<br>
> On 06/15/2012 06:53 PM, Gregory Szorc wrote:<br>
>> On 6/15/12 6:47 AM, Arnaud de Grandmaison wrote:<br>
>>> Hi Manuel & Gregory,<br>
>>><br>
>>> If there are no more feedback, can someone commit those patches for me ?<br>
>>> I do not have commit access.<br>
>> Sorry about the delay.<br>
>><br>
>> I'm not qualified to review the C++. But, the style changes look very<br>
>> good! I'm more than satisfied with its state.<br>
>><br>
>> The Python is *extremely* close. The only major item left (and I didn't<br>
>> mention this in the initial feedback, sorry) is to store a reference to<br>
>> the associated CompilationDatabase in CompileCommands instances. Now,<br>
>> this may not be required (I haven't looked at the C++). If<br>
>> CompileCommands don't internally use a reference to their associated DB,<br>
>> we're fine. But if they do, then it is possible that a garbage<br>
>> collection could collect the DB instance and CompileCommands could blow<br>
>> up on next access. By stuffing a reference to the parent in the child,<br>
>> you prevent GC from prematurely collecting the parent. This manual<br>
>> reference tracking can be done in the from_result(). Just iterate over<br>
>> args until you find an instance of the appropriate type.<br>
>><br>
>> Even if there is no reference now, it may not always work. You should<br>
>> add a test that forces a GC to ensure things don't blow up in the<br>
>> future. Search for "gc" in the test code and you'll find an example.<br>
>><br>
>> Why does CompilationCommands.from_result raise a KeyError? KeyError<br>
>> would be appropriate if you were trying to access an attribute, etc. Its<br>
>> use on a function is IMO inappropriate. If you must raise, I think it<br>
>> should be a regular Exception. But, I think in this case the "Pythonic"<br>
>> thing to do is return None.<br>
>><br>
>> Other nits:<br>
>><br>
>> * I like CompilateDatabase.fromDirectory, not createFromDirectory.<br>
>> * Don't import * in the test. Instead, import specific symbols.<br>
>><br>
>><br>
> CompilationCommands and CompilationDatabase are completely independent<br>
> memory areas, but this may not be the case for CompilationCommand and<br>
> CompilationCommands. I have to check if we get a copy of the string.<br>
> Nevertheless, I will add the gc checks for CompilationCommand(s). Having<br>
> them to catch errors if implementation changes is good.<br>
><br>
> For the KeyError, I wanted to differentiate between existing entries<br>
> (with no associated data) in the database, and non existing keys. Not<br>
> sure if this is really worth though. I am not a python expert, and I<br>
> will gladly make it more pythonic.<br>
><br>
> I will update the patches.<br>
><br>
<br>
<br>
--<br>
Arnaud de Grandmaison<br>
</div></div><div><div>Senior CPU engineer<br>
Business Unit Digital Tuner<br>
<br>
Parrot S.A.<br>
174, quai de Jemmapes<br>
75010 Paris - France<br>
Phone: <a href="tel:%2B33%201%2048%2003%2084%2059" value="+33148038459" target="_blank">+33 1 48 03 84 59</a><br>
<br>
</div></div></blockquote></div><br>
</font></div>