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

[Joshua Cranmer]

On 8/3/2012 4:47 PM, Sean Silva wrote:
>> 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.
> I'm not seeing this. It could be as simple as having each class with
> "callbacks" have a method similar to PP.addPPCallbacks(). The repeated
> loops are not what concerns me. An easy way to accomplish this is to
> document how to add plugin extensibility to a class (something that
> you have to do anyway...); then, starting with an empty callbacks
> struct (or whatever), implement the current functionality
> (DiagnosticClient/ASTConsumer/PPCallbacks) by following that
> documentation you wrote. In fact, adding back the current
> functionality could (probably should) be separate patches.

I don't think I understand what you want here. First you were talking 
about isolating and factoring out the commonality here, and now you're 
saying in effect that I should duplicate the common code to other parts 
of the codebase.

I think I said this before, but I want adding plugin extension points to 
be easy, and it should not require refactoring the point you want to 
extend just to be able to add the hook. The current minor diversity of 
hook points makes it fairly clear that this isn't necessary...
>> 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++
>> :-/ ).
> This is the kind of thing usually solved with a pimpl. Actually, as I
> look at it more closely, I see no reason why initializePlugins()
> should even be exposed in the header since things will break if this
> is ever called other than in the one place where it supposed to be
> called, so a pimpl is probably needed.

I remain unconvinced that a pimpl approach is useful here.
> The loadPlugin interface is broken anyway, as currently nothing is 
> statically enforcing the ordering on method calls for PluginManager. 
> For example, if makes no sense whatsoever to call addPPCallbacks 
> before callOnTUHook, or callOnTUHook before all calls to loadPlugin 
> are done. I think that a redesign is in order to enforce these 
> invariants by design (this is C++: users deserve to have the compiler 
> enforce these things).

While there is nothing enforcing the order on the method calls, I would 
ague that there doesn't need to be any: it is perfectly reasonable 
semantics as it stands right now, even if doing it in that fashion is 
moderately questionable.

-- 
Joshua Cranmer
News submodule owner
DXR coauthor