> 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 <[email protected]> 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 > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
