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

[Joshua Cranmer]

On 8/2/2012 5:44 PM, Sean Silva wrote:
> These are all doing the same thing and in basically the same way. Try
> to isolate and factor this commonality. This will set a clear
> precedent for how to add "observer"-like plugin functionality to a
> class.

Isolating this is impossible without either using lambdas (not yet 
permissible in LLVM/clang), function class boilerplate (*a lot* of 
boiler plate), macros that take expressions with unbound variables as 
parameters (or just other macros), or refactoring every plugin-hookable 
place. The amount of duplicated code is about 4 lines of code; none of 
the possible options (with the possible exception of lambdas) would make 
things significantly easier or clearer considering how large they would 
be. I tried doing this earlier, in the first few versions of my patch; I 
gave up trying to do this long ago since none of the options are 
palatable, and it's just simply not worth it.

> +  ArrayRef<FrontendOptions::CompilerPlugin> CompilerPlugins =
> +    getFrontendOpts().CompilerPlugins;
> +  for (unsigned i = 0, e = CompilerPlugins.size(); i != e; ++i) {
> +    std::string Error;
> +    StringRef Path = CompilerPlugins[i].Plugin;
> +    if (!PluginMgr->loadPlugin(Path, CompilerPlugins[i].Args, Error)) {
> +      getDiagnostics().Report(diag::err_fe_unable_to_load_plugin)
> +        << Path << Error;
> +      return false;
> +    }
> +  }
>
> How about PluginMgr->loadPlugins(getFrontendOpts().CompilerPlugins),
> and move this to PluginManager.cpp? In general, try to obsessively
> reduce the number of plugin-related lines of code that are not in
> PluginManager.{h,cpp}; this will make future maintenance easier. I
> would shoot for 1 line for each logical point of interaction with the
> plugin system.

I expect in the future that there will be other plugin attachment points 
(e.g., something in the driver) that ought to be able to reuse the 
interface. I understand what you're saying, but from an aesthetic design 
point, the plugin manager shouldn't care *how* the arguments to plugins 
are structured; that's the job of the part of the tool that deals with 
the argument parsing (i.e., the CompilerInstance).

> +bool CompilerInstance::initializePlugins() {
> +  assert(!hasPluginManager() && "Already loaded plugins");
> +  PluginMgr.reset(new plugin::PluginManager);
>
> I *really* dislike this pattern which has crept into CompilerInstance.
> There are all these hidden invariants for which the only
> "documentation" is asserts (and who knows how many such asserts are
> missing!). It's extremely error-prone. One way to circumvent this
> could be to have PluginManager have a sane default constructor and
> embed it by value in CompilerInstance; I believe that all the methods
> on PluginManager naturally become noops when there are no plugins
> loaded. This also prevents there from being a bunch of
> "hasPluginManager()" checks littering the source code.
The diagnostics consumer one isn't a no-op (it turns a non-owning client 
to an owning client, but ChainedDiagnosticConsumer doesn't allow me to 
touch this). The downside to your approach is it means that everyone who 
wants to use CompilerInstance has to learn about the PluginManager as 
well (go C++ :-/ ).

> +    ErrorMsg = "Plugin is not compatible with this version of clang";
> +    return false;
>
> This `ErrorMsg` should probably be a real diagnostic and include stuff
> like "your plugin was compiled for version Foo but this Clang is
> version Bar". Also, we discussed the merit of even checking this; is
> it really going to catch bugs? My thinking is that it won't; perhaps
> the most compelling use case is if your system administrator/automated
> package update updates clang without you knowing, but I'm not
> convinced of the usefulness. Also, I think there are better ways of
> doing this particular check than forcing the plugin writer to add
> boilerplate; off the top of my head, you could just #define
> clang_plugin_on_tu to clang_plugin_on_tu_$MAJOR_$MINOR so it will fail
> to load if the version mismatches (or maybe make it a bit more
> explicit that it is a macro by calling it CLANG_PLUGIN_HOOK_FUNC or
> something).
The problem with your suggestion here is that it becomes impossible to 
disambiguate "you mistyped the function hook" from "you compiled with 
the wrong version of clang." One line of boilerplate for versioning is 
very small indeed (compared to other previous approaches).
> I think it is possible to have DynamicLibrary.h included in just
> PluginManager.cpp. It might require some slight architectural changes
> but I think it will lead to an overall better design. I similarly
> think that it's possible for Plugin.h to only be included in
> PluginManager.cpp: I can't see a compelling reason for any other part
> of Clang to have to concern itself with what the low-level Plugin
> interface is.
This is a similar C++-dependency-requirements sucks issue. It's solvable 
if one of the entries ends up introducing a pointer, but none of the 
levels are particularly enticing for introducing pointers.

-- 
Joshua Cranmer
News submodule owner
DXR coauthor