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

[Arnaud ALLARD DE GRANDMAISON]

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 --------------
A non-text attachment was scrubbed...
Name: 0001-cindex-cdb.patch
Type: text/x-patch
Size: 13661 bytes
Desc: 0001-cindex-cdb.patch
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120622/b4e55774/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-cindex-cdb-python.patch
Type: text/x-patch
Size: 10635 bytes
Desc: 0002-cindex-cdb-python.patch
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120622/b4e55774/attachment-0001.bin>