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

[Manuel Klimek]

On Thu, Jun 21, 2012 at 7:32 PM, Arnaud de Grandmaison <
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/20120621/ec84b81c/attachment.html>