[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

[Douglas Gregor]

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.

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

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.

> Added: cfe/trunk/tools/libclang/CIndexCompilationDB.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CIndexCompilationDB.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*.

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?

	- Doug