[cfe-commits] [PATCH] Implement a better plugin API for clang, v2

[Jordan Rose]

On Jun 26, 2012, at 1:25 PM, Joshua Cranmer wrote:

> +def fperplugin_arg : Joined<"-fplugin-arg-">, Group<f_Group>, Flags<[CC1Option]>,
> +  HelpText<"Pass an argument to a plugin">, MetaVarName<"<plugin>-<name>[=<value>]">;

It's really bugging me that the name of the flag doesn't match the typed form. I think fplugin_arg is fine, even though there's more to it than that.


> +extern void clang_plugin_begin_file(llvm::StringRef FileName,
> +                                    const clang::CompilerInstance &CI,
> +                                    clang::plugin::PluginFileCallbacks &Callbacks);

I would still prefer clang_plugin_begin_tu, but maybe for consistency with the rest of Clang your name is better.


> +class Plugin {
> +  llvm::sys::DynamicLibrary PluginLibrary;
> +  plugin::PluginFileCallbacks Callbacks;
> +public:
> +  Plugin(llvm::sys::DynamicLibrary library) : PluginLibrary(library) {}

Still not initializing Callbacks to 0? "PluginLibrary(library), Callbacks()" should do it just fine.


> +  intptr_t getFunctionCall(const char *name) {

Nitpicking: you're not getting the /call/, you're getting the /function/. Or even more precisely you're getting the function /address/.


> +    if (ASTConsumer *Consumer = it->getCallbacks().astConsumer) {
> +      Consumers.push_back(Consumer);
> +    }

LLVM coding style says no braces around single statements in loops and conditionals. This shows up in a couple of places.


> +  std::vector<Plugin> Plugins;

Can this be a SmallVector?


> +  for (unsigned i = 0,
> +        E = Clang->getFrontendOpts().CompilerPlugins.size(); i != E; ++i) {

Recent convention uses "I" and "E"; "i" and "e" would be okay. I don't understand "i" and "E". (Also, maybe you can just use iterators anyway? Not sure if it'd be cleaner or not here.)