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

[Sean Silva]

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

+    CI.getDiagnostics().setClient(PluginMgr.wrapDiagnostics(
+      CI.getDiagnostics().takeClient()));

PluginMgr.addDiagnosticClients(CI.getDiagnostics())?

+ASTConsumer *PluginManager::wrapASTConsumer(ASTConsumer *Original) {
+  SmallVector<ASTConsumer*, 4> Consumers;
+  Consumers.push_back(Original);
+
+  for (SmallVector<Plugin, 4>::iterator it = Plugins.begin();
+       it != Plugins.end(); ++it)
+    if (ASTConsumer *Consumer = it->getCallbacks().ASTCallback)
+      Consumers.push_back(Consumer);
+
+  // Don't construct a multiplex consumer if we're only using the original.
+  return Consumers.size() > 1 ? new MultiplexConsumer(Consumers) : Original;
+}
+
+void PluginManager::addPPCallbacks(Preprocessor &PP) {
+  for (SmallVector<Plugin, 4>::iterator it = Plugins.begin();
+       it != Plugins.end(); ++it) {
+    if (it->getCallbacks().PreprocessorCallback)
+      PP.addPPCallbacks(it->getCallbacks().PreprocessorCallback);
+  }
+}
+
+DiagnosticConsumer *
+PluginManager::wrapDiagnostics(DiagnosticConsumer *Consumer) {
+  for (SmallVector<Plugin, 4>::iterator it = Plugins.begin();
+       it != Plugins.end(); ++it) {
+    if (it->getCallbacks().DiagnosticsCallback)
+      Consumer = new ChainedDiagnosticConsumer(Consumer,
+        it->getCallbacks().DiagnosticsCallback);
+  }
+  return Consumer;
+}

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.

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

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

--- a/examples/PrintFunctionNames/README.txt
+++ b/examples/PrintFunctionNames/README.txt

This README is archaic and needs a lot of love. It should at least
point to the plugin documentation. It's also pretty silly that it
assumes Make and a Debug+Asserts build. Kind of redundant as well
since the plugin mechanism has a tutorial that can be trivially linked
to.

+//===--- PluginManager.h ----------------------------------------*-
C++ -*_===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_PLUGINMANAGER_H
+#define LLVM_CLANG_PLUGINMANAGER_H
+
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/Plugin.h"
+
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/DynamicLibrary.h"

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.

--Sean Silva

2012/8/2 Joshua Cranmer <pidgeot18 at gmail.com>:
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits