[cfe-dev] CompilationDatabase support added to libclang and python binding

[Manuel Klimek]

On Fri, Jun 22, 2012 at 10:41 AM, Arnaud de Grandmaison <
arnaud.allarddegrandmaison at parrot.com> wrote:

>  Thanks for the review.
>
> 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 ?
>

/me didn't notice this is not on cfe-commits :D; yea, post there first.

Otherwise public IRC is always a good way to grab people's attention. Doug,
Argiris or Ted are apparently good approvers according to Chandler ...

Cheers,
/Manuel


>
>
> Cheers,
> --
> Arnaud de Grandmaison
>
> On 06/22/2012 10:28 AM, Manuel Klimek wrote:
>
> 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
> ;).
>
>  Cheers,
> /Manuel
>
> On Fri, Jun 22, 2012 at 1:39 AM, Arnaud ALLARD DE GRANDMAISON <
> arnaud.allarddegrandmaison at parrot.com> wrote:
>
>> Hi Manuel and Gregory
>>
>> I reworked the C/C++ part of the patch as discussed in our previous email.
>>
>> 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.
>>
>> Cheers,
>> --
>> Arnaud de Grandmaison
>>
>>  ________________________________________
>> From: Manuel Klimek [klimek at google.com]
>> Sent: Thursday, June 21, 2012 7:37 PM
>> To: Arnaud ALLARD DE GRANDMAISON
>> Cc: Gregory Szorc; cfe-dev at cs.uiuc.edu
>> Subject: Re: [cfe-dev] CompilationDatabase support added to libclang and
>> python binding
>>
>> On Thu, Jun 21, 2012 at 7:32 PM, Arnaud de Grandmaison <
>> arnaud.allarddegrandmaison at parrot.com<mailto:
>> arnaud.allarddegrandmaison at parrot.com>> wrote:
>> Thanks Manuel for the review. See my comments below.
>>
>>
>> On 06/21/2012 06:21 PM, Manuel Klimek wrote:
>> Review of the C/C++ parts:
>>
>> General comment: are there different style rules for libclang? If not,
>> shouldn't function arguments be FirstLetterCapital?
>>
>> Looking at CIndex.h, we can see a mix of different styles (underscores,
>> camelCase, CamelCase).
>> Example :
>> CINDEX_LINKAGE CXTranslationUnit clang_parseTranslationUnit(CXIndex CIdx,
>>                                                    const char
>> *source_filename
>> ....
>>
>> CINDEX_LINKAGE CXString clang_constructUSR_ObjCMethod(const char *name,
>>                                                      unsigned
>> isInstanceMethod,
>>                                                      CXString classUSR);
>> ....
>>
>>
>> I will fix the patch if there is a preferred style.
>>
>> The style rule is
>> """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)."""
>>
>> Normally the "consistency" rule would win, but if the code is already
>> inconsistent, I'd go with sticking to the style guide.
>>
>>
>>
>> +/**
>> + * \brief Find the compile commands used for a file. The compile commands
>> + * must be freed by \c clang_tooling_CompileCommands_dispose.
>> + */
>> +CINDEX_LINKAGE CXCompileCommands
>>
>> +clang_tooling_CompilationDatabase_getCompileCommands(CXCompilationDatabase,
>> +                                                  const char
>> *completeFileName);
>>
>> Indent off (here and elsewhere)
>>
>> 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.
>>
>> Or break after the opening paren?
>> clang_tooling_CompilationDatabase_getCompileCommands(
>>  CXCompilationDatabase,
>>  const char *completeFileName);
>>
>>
>>
>> +      if (strncmp(argv[i],"lookup", 7)==0){
>>
>> Is there a case where this is different from using (or strlen("lookup"))?
>> I will use strcmp directly.
>>
>>
>>
>> +// FIXME: do something more usefull with the error message
>> +CXCompilationDatabase
>> +clang_tooling_CompilationDatabase_fromDirectory(const char *buildDir)
>> +{
>> +  std::string errorMsg;
>> +
>> +  CompilationDatabase *db =
>> CompilationDatabase::loadFromDirectory(buildDir,
>> +
>> errorMsg);
>> +
>> +  if (!db) {
>> +    fprintf(stderr, "LIBCLANG TOOLING FATAL ERROR: %s\n",
>> errorMsg.c_str());
>> +    ::abort();
>> +  }
>> +
>> +  return db;
>> +}
>>
>> 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 :)
>> Yep. Abort will go away. The caller needs to be able to retry.
>> 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) ?
>>
>> 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.
>>
>>
>>
>>
>> +CXCompileCommands
>> +clang_tooling_CompilationDatabase_getCompileCommands(CXCompilationDatabase
>> CDb,
>> +                                 const char *completeFileName)
>> +{
>> +  if (CompilationDatabase *db = static_cast<CompilationDatabase *>(CDb))
>> {
>> +    const std::vector<CompileCommand>
>> +      CCmd(db->getCompileCommands(completeFileName));
>> +    if (!CCmd.empty())
>> +      return new AllocatedCXCompileCommands( CCmd );
>> +
>>
>> Any reason to not want to have the empty() and error() cases
>> differentiated?
>> 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.
>>
>> Fair enough.
>>
>>
>>
>> +void
>> +clang_tooling_CompileCommands_dispose(CXCompileCommands cmds)
>> +{
>> +  if (cmds)
>> +    delete static_cast<AllocatedCXCompileCommands *>(cmds);
>>
>> isn't the if (cmds) unnecessary?
>>
>> Yes. I will fix it.
>>
>> Cheers,
>> /Manuel
>>
>>
>>
>> Thanks again for the review,
>>
>> --
>> Arnaud de Grandmaison
>>
>>
>>
>
>
> --
> Arnaud de Grandmaison
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120622/a4ed7170/attachment.html>