The patch looks good to me aside from the issues below.

  +//  FIXME: The strategy forward is to provide a plugin system that can
load
  +//  custom compilation databases and make enabling that a build option.

I think this is either a leftover or needs to be adapted to the init stuff.

  +#ifndef LLVM_TOOLS_CLANG_LIB_TOOLING_CUSTOMTOOLINIT_H_

This should be LLVM_CLANG_TOOLING_CUSTOM_TOOL_INIT_H
We leave out TOOLS and LIB, don't put "_" at the end and separate CamelCase
by "_".

  +void customToolInit(int argc, const char **argv);

I think we should put this into the clang::tooling:: namespace, just in
case. Could you then also put the findCompilationDatabaseForDirectory()
into that namespace?

  +#ifndef LLVM_TOOLS_CLANG_LIB_TOOLING_CUSTOMCOMPILATIONDATABASE_H_

Thanks for adding this (/me forgot), but it should be:
LLVM_CLANG_TOOLING_CUSTOM_COMPILATION_DATABASE_H

  +#ifdef USE_CUSTOM_TOOL_INIT
  +#include "clang/Tooling/CustomToolInit.h"
  +#endif

Have you actually tried compiling with that option? I think it can't find
the header file like that as there is no such directory. Simply do
'#include "CustomToolInit.h"'.


On Tue, Jul 17, 2012 at 4:47 PM, Alexander Kornienko <[email protected]>wrote:

> This patch adds a custom initialize hook for clang tools. This is useful
> for plugging in custom initialization in private code bases and IDEs.
> Currently it can be used by compiling with a -DUSE_CUSTOM_TOOL_INIT and
> linking in an implementation for the customToolInit function.
>
> --
> Regards,
> Alexander
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to