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

[Sean Silva]

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

Ideally, all of these classes would have a common base or a traits
class, but the first step to isolating and factoring that is to have a
uniform mechanism for adding callbacks. It would first result in a
little bit of duplicated code, but once done, the means of eliminating
the duplicated code will be obvious; at that point, the duplication
can be easily factored.


> I remain unconvinced that a pimpl approach is useful here.

Ok. This seems to be mostly and aesthetic question, and we disagree.
I'm not going to press on this further unless I come up with some more
concrete improvements (probably in the form of a patch).


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

I disagree. Please make sure that the compiler enforces these
invariants. I promise that besides being safer, the code will also be
more self-documenting. I mean, it's almost like you're choosing to
eschew type safety: you have a class of bugs that the compiler can
guarantee won't happen, yet you are deciding to not leverage the
compiler to prevent them.

--Sean Silva

On Thu, Aug 9, 2012 at 11:36 AM, Joshua Cranmer <pidgeot18 at gmail.com> wrote:
> 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
>