[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 Jul 2, 2012, at 1:39 PM, Arnaud de Grandmaison wrote:

> 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.

Thanks!

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

An example would be helpful. If we add more database formats later, we can update the docs here.

>>> 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.

I was thinking about both: an error code and a string, the latter containing additional information augmenting the error code. We can discuss this in the other thread.

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


I'm actually curious about why you need to extract this information at all for your use case. Typically with code completion in libclang, one simply needs to construct the original command line (which the compilation database can do for us), parse that translation unit, and then code-complete within that translation unit. There's no need to look at or interpret the command-line options.

I was actually referring to something a bit different. If one has a compilation database, then one can presumably use that compilation database as the source of command lines. For example, one could simply have

	clang_codeComplete(CXCompilationDatabase db, const char *filename, unsigned line, unsigned column, unsigned options)

and this routine would query the compilation database to find the appropriate command line for the given filename, automating most of the process. Thinking about it further, this approach would actually be a significant effort, because if we went this way, we'd want to automate a lot more of the process of managing multiple translation units and querying them appropriately. I suspect we'll actually want to tackle this particular issue as part of the Clang service architecture discussed a few weeks back.

	- Doug