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

[Manuel Klimek]

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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120622/76b4fb4f/attachment.html>