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

[Arnaud de Grandmaison]

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.

> +/**
> + * \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.

> +      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) ?

>
> +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.

> +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/20120621/4293baf5/attachment.html>