[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:25
+/// \macro LLVM_PLUGIN_API_VERSION
+/// \brief Identifies the API version understood by this plugin.
+///
----------------
Here and below, we turn on Doxygen's auto-brief feature so I suspect most of the `\brief` tags can be dropped. This one might be the exception, I don't recall if autobrief works for `\macro` blocks or not.


================
Comment at: include/llvm/Passes/PassPlugin.h:55
+/// using the \c LLVM_PLUGIN macro.
+PluginInfo LLVM_ATTRIBUTE_WEAK LLVM_PLUGIN_EXPORT llvmPluginGetInfo();
+}
----------------
A separate question: should this be spelled to be more specific? I'm imagining: `llvmGetPassPluginInfo`. Similar question about the name of the struct as well -- should it be specific to this being a pass plugin?


================
Comment at: include/llvm/Passes/PassPlugin.h:44
+
+#define LLVM_PLUGIN(NAME, PLUGIN_VERSION, REGISTER_CALLBACKS)                  \
+  extern "C" {                                                                 \
----------------
philip.pfaffe wrote:
> chandlerc wrote:
> > Out of curiosity -- why use a macro rather than just documenting that a plugin must define the above function?
> Mostly so that clients don't forget the extern "C". I just thought it was convenient and liked the declarative style.
> 
> If you have strong feelings about this I can document it instead, I'm not really married to the idea.
You can close the block-level `extern "C"` above after the type, and use a direct one here so that copy/paste of the declaration Just Works:

```
extern "C" PluginInfo LLVM_ATTRIBUTE_WEAK LLVM_PLUGIN_EXPORT llvmPluginGetInfo();
```

But maybe better to just give an example definition for copy/paste because I suspect it needn't be so long. At least, I would hope that the declaration here suffices to get the attributes and exports correct and the user can just:

```
extern "C" PluginInfo llvmPluginGetInfo() {
  ...
}
```


I'm always hesitant to generate code with a macro. It feels ... icky to me. Maybe that's just my bias.


================
Comment at: include/llvm/Passes/PassPluginLoader.h:18
+namespace llvm {
+class Plugin {
+public:
----------------
philip.pfaffe wrote:
> chandlerc wrote:
> > This whole class needs doxygen comments. =D
> > 
> > Also, should it be called PassPlugin? And maybe be in the same header as above?
> I put the two classes in different headers because they have different clients. The `PassPlugin.h` header contains what's required on the client side, i.e. in the actual pass plugin. The `Plugin` class here is on the other hand only used by the driver to represent loaded plugins.
I don't (personally) see a big problem with having these in one file. It seems unlikely to be a burden on the plugin author to have this code included and keeps it co-located.

I would try to use names that differentiate the loaded plugin object from the C-struct used by a plugin to describe itself. I'm not sure what the best naming pair is there....


Repository:
  rL LLVM

https://reviews.llvm.org/D35258