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

[Manuel Klimek]

Review of the C/C++ parts:

General comment: are there different style rules for libclang? If not,
shouldn't function arguments be FirstLetterCapital?

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

+      if (strncmp(argv[i],"lookup", 7)==0){

Is there a case where this is different from using (or strlen("lookup"))?

+// 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 :)

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

+void
+clang_tooling_CompileCommands_dispose(CXCompileCommands cmds)
+{
+  if (cmds)
+    delete static_cast<AllocatedCXCompileCommands *>(cmds);

isn't the if (cmds) unnecessary?

Cheers,
/Manuel

On Tue, Jun 19, 2012 at 10:19 AM, Arnaud de Grandmaison <
arnaud.allarddegrandmaison at parrot.com> wrote:

> Hi Manuel & Gregory,
>
> Here are the updated patches.
>
> The only significant change is in the python binding, where a
> CompilationCommand holds a reference to the associated
> CompilationCommands, preventing garbage collection. This is now also
> tested.
>
> Cheers,
> --
> Arnaud de Grandmaison
>
> On 06/18/2012 09:52 AM, Arnaud de Grandmaison wrote:
> > On 06/15/2012 06:53 PM, Gregory Szorc wrote:
> >> On 6/15/12 6:47 AM, Arnaud de Grandmaison wrote:
> >>> Hi Manuel & Gregory,
> >>>
> >>> If there are no more feedback, can someone commit those patches for me
> ?
> >>> I do not have commit access.
> >> Sorry about the delay.
> >>
> >> I'm not qualified to review the C++. But, the style changes look very
> >> good! I'm more than satisfied with its state.
> >>
> >> The Python is *extremely* close. The only major item left (and I didn't
> >> mention this in the initial feedback, sorry) is to store a reference to
> >> the associated CompilationDatabase in CompileCommands instances. Now,
> >> this may not be required (I haven't looked at the C++). If
> >> CompileCommands don't internally use a reference to their associated DB,
> >> we're fine. But if they do, then it is possible that a garbage
> >> collection could collect the DB instance and CompileCommands could blow
> >> up on next access. By stuffing a reference to the parent in the child,
> >> you prevent GC from prematurely collecting the parent. This manual
> >> reference tracking can be done in the from_result(). Just iterate over
> >> args until you find an instance of the appropriate type.
> >>
> >> Even if there is no reference now, it may not always work. You should
> >> add a test that forces a GC to ensure things don't blow up in the
> >> future. Search for "gc" in the test code and you'll find an example.
> >>
> >> Why does CompilationCommands.from_result raise a KeyError? KeyError
> >> would be appropriate if you were trying to access an attribute, etc. Its
> >> use on a function is IMO inappropriate. If you must raise, I think it
> >> should be a regular Exception. But, I think in this case the "Pythonic"
> >> thing to do is return None.
> >>
> >> Other nits:
> >>
> >> * I like CompilateDatabase.fromDirectory, not createFromDirectory.
> >> * Don't import * in the test. Instead, import specific symbols.
> >>
> >>
> > CompilationCommands and CompilationDatabase are completely independent
> > memory areas, but this may not be the case for CompilationCommand and
> > CompilationCommands. I have to check if we get a copy of the string.
> > Nevertheless, I will add the gc checks for CompilationCommand(s). Having
> > them to catch errors if implementation changes is good.
> >
> > For the KeyError, I wanted to differentiate between existing entries
> > (with no associated data) in the database, and non existing keys. Not
> > sure if this is really worth though. I am not a python expert, and I
> > will gladly make it more pythonic.
> >
> > I will update the patches.
> >
>
>
> --
> Arnaud de Grandmaison
> Senior CPU engineer
> Business Unit Digital Tuner
>
> Parrot S.A.
> 174, quai de Jemmapes
> 75010 Paris - France
> Phone: +33 1 48 03 84 59
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120621/51caebe9/attachment.html>