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"
<[email protected]> 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>From 0d78f34d3c0614a31eeedaaf55367c569827ae31 Mon Sep 17 00:00:00 2001
From: "Arnaud A. de Grandmaison" <[email protected]>
Date: Mon, 2 Jul 2012 22:36:16 +0200
Subject: [PATCH] [libclang] CompilationDatabase naming and comment fixes
---
bindings/python/clang/cindex.py | 18 ++++++------
include/clang-c/CXCompilationDatabase.h | 41 ++++++++++++++++--------------
tools/c-index-test/c-index-test.c | 18 ++++++------
tools/libclang/CXCompilationDatabase.cpp | 23 ++++++++--------
tools/libclang/libclang.exports | 18 ++++++------
5 files changed, 60 insertions(+), 58 deletions(-)
diff --git a/bindings/python/clang/cindex.py b/bindings/python/clang/cindex.py
index 23c1ede..1a471cd 100644
--- a/bindings/python/clang/cindex.py
+++ b/bindings/python/clang/cindex.py
@@ -2578,39 +2578,39 @@ _clang_getCompletionPriority.argtypes = [c_void_p]
_clang_getCompletionPriority.restype = c_int
# Compilation Database
-CompilationDatabase_fromDirectory = lib.clang_tooling_CompilationDatabase_fromDirectory
+CompilationDatabase_fromDirectory = lib.clang_CompilationDatabase_fromDirectory
CompilationDatabase_fromDirectory.argtypes = [c_char_p, POINTER(c_uint)]
CompilationDatabase_fromDirectory.restype = c_object_p
CompilationDatabase_fromDirectory.errcheck = CompilationDatabase.from_result
-CompilationDatabase_dispose = lib.clang_tooling_CompilationDatabase_dispose
+CompilationDatabase_dispose = lib.clang_CompilationDatabase_dispose
CompilationDatabase_dispose.argtypes = [c_object_p]
-CompilationDatabase_getCompileCommands = lib.clang_tooling_CompilationDatabase_getCompileCommands
+CompilationDatabase_getCompileCommands = lib.clang_CompilationDatabase_getCompileCommands
CompilationDatabase_getCompileCommands.argtypes = [c_object_p, c_char_p]
CompilationDatabase_getCompileCommands.restype = c_object_p
CompilationDatabase_getCompileCommands.errcheck = CompileCommands.from_result
-CompileCommands_dispose = lib.clang_tooling_CompileCommands_dispose
+CompileCommands_dispose = lib.clang_CompileCommands_dispose
CompileCommands_dispose.argtypes = [c_object_p]
-CompileCommands_getSize = lib.clang_tooling_CompileCommands_getSize
+CompileCommands_getSize = lib.clang_CompileCommands_getSize
CompileCommands_getSize.argtypes = [c_object_p]
CompileCommands_getSize.restype = c_uint
-CompileCommands_getCommand = lib.clang_tooling_CompileCommands_getCommand
+CompileCommands_getCommand = lib.clang_CompileCommands_getCommand
CompileCommands_getCommand.argtypes = [c_object_p, c_uint]
CompileCommands_getCommand.restype = c_object_p
-CompileCommand_getDirectory = lib.clang_tooling_CompileCommand_getDirectory
+CompileCommand_getDirectory = lib.clang_CompileCommand_getDirectory
CompileCommand_getDirectory.argtypes = [c_object_p]
CompileCommand_getDirectory.restype = _CXString
-CompileCommand_getNumArgs = lib.clang_tooling_CompileCommand_getNumArgs
+CompileCommand_getNumArgs = lib.clang_CompileCommand_getNumArgs
CompileCommand_getNumArgs.argtypes = [c_object_p]
CompileCommand_getNumArgs.restype = c_uint
-CompileCommand_getArg = lib.clang_tooling_CompileCommand_getArg
+CompileCommand_getArg = lib.clang_CompileCommand_getArg
CompileCommand_getArg.argtypes = [c_object_p, c_uint]
CompileCommand_getArg.restype = _CXString
diff --git a/include/clang-c/CXCompilationDatabase.h b/include/clang-c/CXCompilationDatabase.h
index e4da627..d11133c 100644
--- a/include/clang-c/CXCompilationDatabase.h
+++ b/include/clang-c/CXCompilationDatabase.h
@@ -29,9 +29,11 @@ extern "C" {
*/
/**
- * \brief Represents clang::tooling::CompilationDatabase
+ * A compilation database holds all information used to compile files in a
+ * project. For each file in the database, it can be queried for the working
+ * directory or the command line used for the compiler invocation.
*
- * Must be freed by \c clang_tooling_CompilationDatabase_dispose
+ * Must be freed by \c clang_CompilationDatabase_dispose
*/
typedef void * CXCompilationDatabase;
@@ -42,7 +44,7 @@ typedef void * CXCompilationDatabase;
* return several commands, as the file may have been compiled with
* different options in different places of the project. This choice of compile
* commands is wrapped in this opaque data structure. It must be freed by
- * \c clang_tooling_CompileCommands_dispose.
+ * \c clang_CompileCommands_dispose.
*/
typedef void * CXCompileCommands;
@@ -69,59 +71,60 @@ typedef enum {
/**
* \brief Creates a compilation database from the database found in directory
- * buildDir. It must be freed by \c clang_tooling_CompilationDatabase_dispose.
+ * buildDir. For example, CMake can output a compile_commands.json which can
+ * be used to build the database.
+ *
+ * It must be freed by \c clang_CompilationDatabase_dispose.
*/
CINDEX_LINKAGE CXCompilationDatabase
-clang_tooling_CompilationDatabase_fromDirectory(
- const char *BuildDir,
- CXCompilationDatabase_Error *ErrorCode);
+clang_CompilationDatabase_fromDirectory(const char *BuildDir,
+ CXCompilationDatabase_Error *ErrorCode);
/**
* \brief Free the given compilation database
*/
CINDEX_LINKAGE void
-clang_tooling_CompilationDatabase_dispose(CXCompilationDatabase);
+clang_CompilationDatabase_dispose(CXCompilationDatabase);
/**
* \brief Find the compile commands used for a file. The compile commands
- * must be freed by \c clang_tooling_CompileCommands_dispose.
+ * must be freed by \c clang_CompileCommands_dispose.
*/
CINDEX_LINKAGE CXCompileCommands
-clang_tooling_CompilationDatabase_getCompileCommands(
- CXCompilationDatabase,
- const char *CompleteFileName);
+clang_CompilationDatabase_getCompileCommands(CXCompilationDatabase,
+ const char *CompleteFileName);
/**
* \brief Free the given CompileCommands
*/
-CINDEX_LINKAGE void clang_tooling_CompileCommands_dispose(CXCompileCommands);
+CINDEX_LINKAGE void clang_CompileCommands_dispose(CXCompileCommands);
/**
* \brief Get the number of CompileCommand we have for a file
*/
CINDEX_LINKAGE unsigned
-clang_tooling_CompileCommands_getSize(CXCompileCommands);
+clang_CompileCommands_getSize(CXCompileCommands);
/**
* \brief Get the I'th CompileCommand for a file
*
- * Note : 0 <= i < clang_tooling_CompileCommands_getSize(CXCompileCommands)
+ * Note : 0 <= i < clang_CompileCommands_getSize(CXCompileCommands)
*/
CINDEX_LINKAGE CXCompileCommand
-clang_tooling_CompileCommands_getCommand(CXCompileCommands, unsigned I);
+clang_CompileCommands_getCommand(CXCompileCommands, unsigned I);
/**
* \brief Get the working directory where the CompileCommand was executed from
*/
CINDEX_LINKAGE CXString
-clang_tooling_CompileCommand_getDirectory(CXCompileCommand);
+clang_CompileCommand_getDirectory(CXCompileCommand);
/**
* \brief Get the number of arguments in the compiler invocation.
*
*/
CINDEX_LINKAGE unsigned
-clang_tooling_CompileCommand_getNumArgs(CXCompileCommand);
+clang_CompileCommand_getNumArgs(CXCompileCommand);
/**
* \brief Get the I'th argument value in the compiler invocations
@@ -130,7 +133,7 @@ clang_tooling_CompileCommand_getNumArgs(CXCompileCommand);
* - argument 0 is the compiler executable
*/
CINDEX_LINKAGE CXString
-clang_tooling_CompileCommand_getArg(CXCompileCommand, unsigned I);
+clang_CompileCommand_getArg(CXCompileCommand, unsigned I);
/**
* @}
diff --git a/tools/c-index-test/c-index-test.c b/tools/c-index-test/c-index-test.c
index 787671f..adf3201 100644
--- a/tools/c-index-test/c-index-test.c
+++ b/tools/c-index-test/c-index-test.c
@@ -2398,7 +2398,7 @@ perform_test_compilation_db(const char *database, int argc, const char **argv) {
memcpy(tmp, database, len+1);
buildDir = dirname(tmp);
- db = clang_tooling_CompilationDatabase_fromDirectory(buildDir, &ec);
+ db = clang_CompilationDatabase_fromDirectory(buildDir, &ec);
if (db) {
@@ -2410,7 +2410,7 @@ perform_test_compilation_db(const char *database, int argc, const char **argv) {
for (i=0; i<argc && errorCode==0; ) {
if (strcmp(argv[i],"lookup")==0){
- CCmds = clang_tooling_CompilationDatabase_getCompileCommands(db, argv[i+1]);
+ CCmds = clang_CompilationDatabase_getCompileCommands(db, argv[i+1]);
if (!CCmds) {
printf("file %s not found in compilation db\n", argv[i+1]);
@@ -2418,7 +2418,7 @@ perform_test_compilation_db(const char *database, int argc, const char **argv) {
break;
}
- numCmds = clang_tooling_CompileCommands_getSize(CCmds);
+ numCmds = clang_CompileCommands_getSize(CCmds);
if (numCmds==0) {
fprintf(stderr, "should not get an empty compileCommand set for file"
@@ -2428,29 +2428,29 @@ perform_test_compilation_db(const char *database, int argc, const char **argv) {
}
for (j=0; j<numCmds; ++j) {
- CCmd = clang_tooling_CompileCommands_getCommand(CCmds, j);
+ CCmd = clang_CompileCommands_getCommand(CCmds, j);
- wd = clang_tooling_CompileCommand_getDirectory(CCmd);
+ wd = clang_CompileCommand_getDirectory(CCmd);
printf("workdir:'%s'", clang_getCString(wd));
clang_disposeString(wd);
printf(" cmdline:'");
- numArgs = clang_tooling_CompileCommand_getNumArgs(CCmd);
+ numArgs = clang_CompileCommand_getNumArgs(CCmd);
for (a=0; a<numArgs; ++a) {
if (a) printf(" ");
- arg = clang_tooling_CompileCommand_getArg(CCmd, a);
+ arg = clang_CompileCommand_getArg(CCmd, a);
printf("%s", clang_getCString(arg));
clang_disposeString(arg);
}
printf("'\n");
}
- clang_tooling_CompileCommands_dispose(CCmds);
+ clang_CompileCommands_dispose(CCmds);
i += 2;
}
}
- clang_tooling_CompilationDatabase_dispose(db);
+ clang_CompilationDatabase_dispose(db);
} else {
printf("database loading failed with error code %d.\n", ec);
errorCode = -1;
diff --git a/tools/libclang/CXCompilationDatabase.cpp b/tools/libclang/CXCompilationDatabase.cpp
index a537c9d..7bd319a 100644
--- a/tools/libclang/CXCompilationDatabase.cpp
+++ b/tools/libclang/CXCompilationDatabase.cpp
@@ -10,9 +10,8 @@ extern "C" {
// FIXME: do something more usefull with the error message
CXCompilationDatabase
-clang_tooling_CompilationDatabase_fromDirectory(
- const char *BuildDir,
- CXCompilationDatabase_Error *ErrorCode)
+clang_CompilationDatabase_fromDirectory(const char *BuildDir,
+ CXCompilationDatabase_Error *ErrorCode)
{
std::string ErrorMsg;
CXCompilationDatabase_Error Err = CXCompilationDatabase_NoError;
@@ -32,7 +31,7 @@ clang_tooling_CompilationDatabase_fromDirectory(
}
void
-clang_tooling_CompilationDatabase_dispose(CXCompilationDatabase CDb)
+clang_CompilationDatabase_dispose(CXCompilationDatabase CDb)
{
delete static_cast<CompilationDatabase *>(CDb);
}
@@ -47,8 +46,8 @@ struct AllocatedCXCompileCommands
};
CXCompileCommands
-clang_tooling_CompilationDatabase_getCompileCommands(CXCompilationDatabase CDb,
- const char *CompleteFileName)
+clang_CompilationDatabase_getCompileCommands(CXCompilationDatabase CDb,
+ const char *CompleteFileName)
{
if (CompilationDatabase *db = static_cast<CompilationDatabase *>(CDb)) {
const std::vector<CompileCommand>
@@ -61,13 +60,13 @@ clang_tooling_CompilationDatabase_getCompileCommands(CXCompilationDatabase CDb,
}
void
-clang_tooling_CompileCommands_dispose(CXCompileCommands Cmds)
+clang_CompileCommands_dispose(CXCompileCommands Cmds)
{
delete static_cast<AllocatedCXCompileCommands *>(Cmds);
}
unsigned
-clang_tooling_CompileCommands_getSize(CXCompileCommands Cmds)
+clang_CompileCommands_getSize(CXCompileCommands Cmds)
{
if (!Cmds)
return 0;
@@ -79,7 +78,7 @@ clang_tooling_CompileCommands_getSize(CXCompileCommands Cmds)
}
CXCompileCommand
-clang_tooling_CompileCommands_getCommand(CXCompileCommands Cmds, unsigned I)
+clang_CompileCommands_getCommand(CXCompileCommands Cmds, unsigned I)
{
if (!Cmds)
return 0;
@@ -94,7 +93,7 @@ clang_tooling_CompileCommands_getCommand(CXCompileCommands Cmds, unsigned I)
}
CXString
-clang_tooling_CompileCommand_getDirectory(CXCompileCommand CCmd)
+clang_CompileCommand_getDirectory(CXCompileCommand CCmd)
{
if (!CCmd)
return createCXString((const char*)NULL);
@@ -104,7 +103,7 @@ clang_tooling_CompileCommand_getDirectory(CXCompileCommand CCmd)
}
unsigned
-clang_tooling_CompileCommand_getNumArgs(CXCompileCommand CCmd)
+clang_CompileCommand_getNumArgs(CXCompileCommand CCmd)
{
if (!CCmd)
return 0;
@@ -113,7 +112,7 @@ clang_tooling_CompileCommand_getNumArgs(CXCompileCommand CCmd)
}
CXString
-clang_tooling_CompileCommand_getArg(CXCompileCommand CCmd, unsigned Arg)
+clang_CompileCommand_getArg(CXCompileCommand CCmd, unsigned Arg)
{
if (!CCmd)
return createCXString((const char*)NULL);
diff --git a/tools/libclang/libclang.exports b/tools/libclang/libclang.exports
index bba883f..28519b5 100644
--- a/tools/libclang/libclang.exports
+++ b/tools/libclang/libclang.exports
@@ -204,14 +204,14 @@ clang_saveTranslationUnit
clang_sortCodeCompletionResults
clang_toggleCrashRecovery
clang_tokenize
-clang_tooling_CompilationDatabase_fromDirectory
-clang_tooling_CompilationDatabase_dispose
-clang_tooling_CompilationDatabase_getCompileCommands
-clang_tooling_CompileCommands_dispose
-clang_tooling_CompileCommands_getSize
-clang_tooling_CompileCommands_getCommand
-clang_tooling_CompileCommand_getDirectory
-clang_tooling_CompileCommand_getNumArgs
-clang_tooling_CompileCommand_getArg
+clang_CompilationDatabase_fromDirectory
+clang_CompilationDatabase_dispose
+clang_CompilationDatabase_getCompileCommands
+clang_CompileCommands_dispose
+clang_CompileCommands_getSize
+clang_CompileCommands_getCommand
+clang_CompileCommand_getDirectory
+clang_CompileCommand_getNumArgs
+clang_CompileCommand_getArg
clang_visitChildren
clang_visitChildrenWithBlock
--
1.7.8.6
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
