[PATCH] D35258: [Plugins] Add a slim plugin API to work together with the new PM

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: include/llvm/Passes/PassPlugin.h:23
+
+#define LLVM_PLUGIN_API_VERSION 1
+
----------------
We should have some documentation around when/where this should be incremented...


================
Comment at: include/llvm/Passes/PassPlugin.h:25-29
+#ifdef WIN32
+#define LLVM_PLUGIN_EXPORT __declspec(dllexport)
+#else
+#define LLVM_PLUGIN_EXPORT
+#endif
----------------
We should probably stick this into Compiler.h


================
Comment at: include/llvm/Passes/PassPlugin.h:40
+
+PluginInfo LLVM_PLUGIN_EXPORT llvmPluginGetInfo();
+}
----------------
Probably some documentation here would be good. ;]

Also, should this be weak so that it is OK if we never load a library that defines it?


================
Comment at: include/llvm/Passes/PassPlugin.h:44
+
+#define LLVM_PLUGIN(NAME, PLUGIN_VERSION, REGISTER_CALLBACKS)                  \
+  extern "C" {                                                                 \
----------------
Out of curiosity -- why use a macro rather than just documenting that a plugin must define the above function?


================
Comment at: include/llvm/Passes/PassPluginLoader.h:18
+namespace llvm {
+class Plugin {
+public:
----------------
This whole class needs doxygen comments. =D

Also, should it be called PassPlugin? And maybe be in the same header as above?


================
Comment at: lib/Passes/PassPluginLoader.cpp:37-39
+    errs() << "Wrong API version on plugin '" << Filename << "'. Got version "
+           << P.Info.APIVersion << ", supported version is "
+           << LLVM_PLUGIN_API_VERSION << ".\n";
----------------
I think this is indicative that optional isn't the right return type here... I think you want something more like llvm::Expected  here so you can put the error message into return and make this more library friendly.


================
Comment at: tools/opt/NewPMDriver.cpp:47
+static cl::list<std::string>
+    PassPlugins("load-plugin", cl::desc("Load passes from plugin library"));
+
----------------
maybe `load-pass-plugin`?


Repository:
  rL LLVM

https://reviews.llvm.org/D35258