[cfe-dev] CompilationDatabase support added to libclang and python binding
Arnaud de Grandmaison
arnaud.allarddegrandmaison at parrot.com
Fri Jun 22 01:41:08 PDT 2012
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 ?
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
> <mailto: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 <mailto: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 <mailto: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><mailto: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/7f19d2c5/attachment.html>
More information about the cfe-dev
mailing list