[cfe-commits] r159484 - in /cfe/trunk: include/clang-c/CXCompilationDatabase.h test/Index/compile_commands.json tools/c-index-test/c-index-test.c tools/libclang/CIndexCompilationDB.cpp tools/libclang/CMakeLists.txt tools/libclang/libclang.exports

[Arnaud de Grandmaison]

Hi Doug,

Thanks for your comments.

See my comments below.

Cheers,
--
Arnaud de Grandmaison

On Monday 02 July 2012 10:33:46 Douglas Gregor wrote:
> On Jun 30, 2012, at 4:27 AM, "Arnaud A. de Grandmaison" 
<arnaud.adegm at gmail.com> wrote:
> > Author: aadg
> > Date: Sat Jun 30 06:27:57 2012
> > New Revision: 159484
> > 
> > URL: http://llvm.org/viewvc/llvm-project?rev=159484&view=rev
> > Log:
> > [libclang] add CompilationDatabase support
> 
> Nifty. Some comments below.
> 
> > +/**
> > + * \brief Represents clang::tooling::CompilationDatabase
> > + *
> > + * Must be freed by \c clang_tooling_CompilationDatabase_dispose
> > + */
> > +typedef void * CXCompilationDatabase;
> 
> libclang comments should be as standalone as possible. Please expand the
> \brief comment to state what a compilation database actually does.

OK, this is expanded in the attached patch.

> > +/**
> > + * \brief Creates a compilation database from the database found in
> > directory + * buildDir. It must be freed by \c
> > clang_tooling_CompilationDatabase_dispose. + */
> > +CINDEX_LINKAGE CXCompilationDatabase
> > +clang_tooling_CompilationDatabase_fromDirectory(
> > +  const char *BuildDir,
> > +  CXCompilationDatabase_Error *ErrorCode);
> 
> Comment should say what can be in buildDir for this to work; it's not simply
> an arbitrary directory.

Well, Manuel did not want me to be too specific here, as different database 
format could be added in the future. But I can at least  give the *example* of 
compile_commands.json. This is in the attached patch.

> 
> Why did you decide on the clang_tooling_ prefix? Tooling isn't something
> that will ever be exposed through libclang; the point of the exposing the
> compilation database is that it makes it easier to work with libclang
> itself.

I wanted to expose the origin of this part of the code --- a kind of 
namespace. But it is true that for the end user of libclang, they do not care 
: the CompilationDatabase is just here as a helper. This is in the patch.

> > Added: cfe/trunk/tools/libclang/CIndexCompilationDB.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CIndexCompil
> > ationDB.cpp?rev=159484&view=auto
> > =========================================================================
> > ===== --- cfe/trunk/tools/libclang/CIndexCompilationDB.cpp (added)
> > +++ cfe/trunk/tools/libclang/CIndexCompilationDB.cpp Sat Jun 30 06:27:57
> > 2012 @@ -0,0 +1,130 @@
> > +#include "clang-c/CXCompilationDatabase.h"
> > +#include "clang/Tooling/CompilationDatabase.h"
> > +#include "CXString.h"
> > +
> > +using namespace clang;
> > +using namespace clang::tooling;
> > +using namespace clang::cxstring;
> > +
> > +extern "C" {
> > +
> > +// FIXME: do something more usefull with the error message
> > +CXCompilationDatabase
> > +clang_tooling_CompilationDatabase_fromDirectory(
> > +  const char *BuildDir,
> > +  CXCompilationDatabase_Error *ErrorCode)
> > +{
> > +  std::string ErrorMsg;
> > +  CXCompilationDatabase_Error Err = CXCompilationDatabase_NoError;
> > +
> > +  CompilationDatabase *db =
> > CompilationDatabase::loadFromDirectory(BuildDir, +                       
> >                                            ErrorMsg); +
> > +  if (!db) {
> > +    fprintf(stderr, "LIBCLANG TOOLING ERROR: %s\n", ErrorMsg.c_str());
> > +    Err = CXCompilationDatabase_CanNotLoadDatabase;
> > +  }
> > +
> > +  if (ErrorCode)
> > +    *ErrorCode = Err;
> > +
> > +  return db;
> > +}
> 
> We can't have the fprintf here; libclang gets linked into applications that
> don't have/need a terminal for standard out or standard error. The error
> message could be passed based via a CXString*.

Good point. I can fix this, but in this case, this should also be be fixed in 
numerous places in CIndex / Indexing.cpp. For the user, I am not sure the 
CXString * is a good option. The user should not have to parse this string to 
understand the error and try to fix it : this is why I added the ErrorCode 
pointer. This is *not* in the attached patch.

> My one general comment here is that the compilation database support is
> completely disjoint from all of the APIs that could benefit from it, so
> it's not really providing any benefit now. Are you planning to add APIs
> based on the compilation database that eliminate the need to pass long
> command lines?

I found out that the command line is a bit too long and needs to be filtered in 
the application. For now, the cursor is on one end of the spectrum : a raw 
table with all options. On the opposite end, there is a full blown command 
line parser, which can be queried to get the options of "interest". This would 
be good to have for the project in the end, but not that easy to do.

My first use for it is for the clang_complete vim plugin. You can get a 
clang_complete with CompilationDatabase support from my cdb branch on 
https://github.com/Arnaud-de-Grandmaison/clang_complete.git. I have not asked 
to merge it upstream, as I want to use it as real life test case : this always 
helps when working on an api.

Based on the above testcase, my suggestion would be to place the cursor 
somewhere in the middle, and beside the full command line, providing accessors 
to :
 1) the compiler executable
 2) the ouput file name
 3) the input file name
 4) the action, i.e. : -c, -S, ...
 5) the include pathes
 6) the defines
 7) arch specific things which can impact the frontend ?
 8) all the other options not caught in the previous steps

I think this should be part of Tooling::CompilationDatabase, and libclang 
should provide a C access to those accessors.

Do you have any ideas / opinion there ?

> 
> 	- Doug
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libclang-CompilationDatabase-naming-and-comment-fixe.patch
Type: text/x-patch
Size: 14229 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120702/809ecc1e/attachment.bin>