Community
Participate
Working Groups
Created attachment 145202 [details] Newsgroup thread It would be useful for the EMF-generated switch classes to be readily composable. Then you could say something like: Switch<T> switch1 = new XxxSwitch<T>() { ... } Switch<T> switch2 = new YyySwitch<T>() { ... } Switch<T> composedSwitch = new ComposedSwitch<T>(); composedSwitch.addSwitch(switch1); composedSwitch.addSwitch(switch2); T result = composedSwitch.doSwitch(...); See the attached newsgroup thread. I have attached an implementation and unit test. Adrian Price Senior Architect TIBCO Software Inc.
Created attachment 145203 [details] Implementation, template, test case Proposed implementation for review.
Created attachment 163816 [details] Patches to address the issue Adrian, I made pretty extensive changes. I also updated all the copyright notices and to get them to refer to the proper EPL license. Please review them as indicate whether you agree with the copyright attribution as I wrote them. One further thing I'm tempted to change is to eliminate the use of synchronization. I think adding and removing really shouldn't need synchronization because it just doesn't make much sense to try to update the composed switch simultaneously on multiple threads. To ensure that the composed switch is thread safe for usage, I'd create a new registry every time an entry needs to be added. This might result in race conditions, but the winner of the race won't matter. The advantage being that usage across multiple threads can be fully concurrent without blocking. What do you think?
Dave/Kenn, Your review would be appreciated.
(In reply to comment #3) > Dave/Kenn, > > Your review would be appreciated. It looks good to me, at a glance, except for an extraneous "appropriate" and a missing hyphen between "non" and "null" in the Javadoc comments for the new Switch class.
[I have a hard time understanding how this proposal works by reading the patch] Some time ago I created an extensible switch that works with the existing emf switches by using an interface ISwitch: /** * * @author scharf * * This is a extensible switch class modeled after the EMF switch. * * @param <T> the return type of the switch * @param <C> the type of context data shared between the switches */ public interface ISwitch<T,C> { /** * @param context information shared between distinct switch classes * @param rootSwitch call {@link ISwitch#handleSwitch(Object)} whenyou want to * dispatch an object */ void init(C context,ISwitch<T, C> rootSwitch); /** * @param object * @return false if this switch cannot handle the object */ boolean isSwitchFor(Object object); /** * @param object * @return the value or <code>null</null> if the class cannot handle the object. */ T handleSwitch(Object object); /** * Calls a switch on the root object * @param object * @return null or the result of the switch */ T doRootSwitch(Object object); } My switch includes the notion of a context: that is data shared between the switches. If a switch is a single class, then the instance can contain data that is used caseXxx calls but if the switch is composed there has to be a way to share data. My switch is not confined to EObjects: it can also handle non EObjects (but that is a convenience for my use case) Switches that are composed have to be able to call into the root switch. An extensible switch is created like this (where XxxxSwitch is the normal emf switch): public class XxxxExtensibleSwitch<T,C> extends XxxxSwitch<T> implements ISwitch<T, C>{ private ISwitch<T, C> fRootSwitch; protected C fContext; public XxxxExtensibleSwitch() { // Make sure this switch can run stand alone fRootSwitch=this; } public T handleSwitch(Object object) { return super.doSwitch((EObject)object); } public void init(C context, ISwitch<T, C> rootSwitch) { fRootSwitch=rootSwitch; fContext=context; } public boolean isSwitchFor(Object object) { if(object instanceof EObject) // IMPORTANT if you copy this code: replace EcorePackage.eINSTANCE with your package return ((EObject)object).eClass().getEPackage()==IXxxxPackage.eINSTANCE; return false; } @Override final public T doSwitch(EObject theEObject) { return fRootSwitch.handleSwitch(theEObject); } public T doRootSwitch(Object object) { return fRootSwitch.handleSwitch(object); } } See also https://www.eclipsecon.org/submissions/2010/view_talk.php?id=1480
(In reply to comment #2) > I made pretty extensive changes. I also updated all the copyright notices and > to get them to refer to the proper EPL license. Please review them as indicate > whether you agree with the copyright attribution as I wrote them. Thanks for the review and changes Ed. If it's okay with you I think TIBCO would prefer their copyright attribution as "TIBCO Software Inc." (note casing). Your other changes are fine by me. > One further thing I'm tempted to change is to eliminate the use of > synchronization. I think adding and removing really shouldn't need > synchronization because it just doesn't make much sense to try to update the > composed switch simultaneously on multiple threads. To ensure that the > composed switch is thread safe for usage, I'd create a new registry every time > an entry needs to be added. This might result in race conditions, but the > winner of the race won't matter. The advantage being that usage across multiple > threads can be fully concurrent without blocking. What do you think? My guess is that such dynamic cases would be rather rare; in fact not one of our own use cases involves dynamically changing switch composition. I totally agree with your proposed change: let's remove all the synchronization and reallocate registry if/when it needs to change; as you say, it matters not which thread wins any race. Good catch Ken: Switch.java(47): "... one returns a non null ..." ^ Switch.java(68): "... the appropriate appropriate <code>caseXXX</code> ..." ^^^^^^^^^^^ By the way, were the test cases inappropriate in some way? They weren't present in your patch. Cheers, --A
(In reply to comment #5) Michael, thanks for the input. > [I have a hard time understanding how this proposal works by reading the patch] The basic design originated in the attached newsgroup thread. Other than that I'm afraid you'd need to apply the patch to the source code :-) > My switch includes the notion of a context: that is data shared between the > switches. I've always handled a fixed switch context as instance state in the switch subclass as you do. However, there are many examples of dynamic switch context such as status objects, loggers, progress monitors, result aggregators, etc. but the current caseXxx(obj) signature makes such switches non-reusable - one must either allocate a new switch per context or pass the context via ThreadLocal data (a heavyweight solution). I suppose one could use pooling techniques but I often think a threadsafe singleton would be preferable, passing dynamic context as an additional argument to doSwitch(obj, context) and thence to caseXxx(obj, context) methods. Cheers, --A
What I do not understand is: 1. How context can be shared between switches in the proposed solution? My solution allows to set the context (recursively). For a single switch the managing of context is easy because it can be kept as a member. For a composite switch there has to be a way to share the context. I do not buy the argument that you need to share switches between threads. Switches are relatively lightweight and what is the problem in creating a new (composite) switch for each thread. 2. How can a child switch call the root (=composite) switch? My solution the XxxxSwitch.doSwitch method which calls the "root switch" (which is the compoiste switch) and that does then dispatch down. (Looking at the patch) it seems that there is no way to call the parent switch once you are in one of the leaves...
Michael, Given "Some time ago I created an extensible switch that works with the existing emf switches by using an interface ISwitch" doesn't that comment continue to apply? I imagine there are other ways to pass context, e.g., an outer composed switch class which has a field for the context, and inner classes for all the other switches which can see that field. Given "Switches are relatively lightweight", what's the problem of creating a context specific one?
Ed, sure my solution still works. I just wonder what is the value of a composite switch that does not provide a way to access a shared context. Any switch that acts like a builder usually requires a context. It is easy to create switches that keep the context. Hmm, I created an extension point to register contributions to composite switches. That is why I needed a way to pass the context down to the children. If the switches are composed programmatically (or using dependency injection) passing down the context is not needed... The other question is still open for me: how does a child switch call doSwitch on the root composite? When a child switch calls doSwitch it will only consult the caseXxx on that instance of the switch. But it should go up to the root composite to be able to handle objects that are not defined in the package of the current switch. Or am I missing something?
Dependency injection seems to be a popular technique these days. I imagine that if one is writing a switch that depends on context, that one could do that in any number of ways, only one of which is passing the context to all the calls. I assume you're talking about recursive switching? Again, inner classes could nicely call methods on the root composing switch. Perhaps given that we're talking at a very high abstract level, it's impossible to say much concrete, leading to discussions that just circle...
Adrian, I'll make the copyright changes and include a new patch. The test case was really good and I'll definitely include it; I must have missed it from the patch. Given the size of the contribution, I'll need to file a CQ. I'd like to do that once I'm sure the final details are settled. Hopefully Dave will have time to respond.
Ed, to get things more concrete, I'd wait until it is checked in and I'd create an example/test that would show the problem of not calling the parent doSwitch
Created attachment 164129 [details] Updates Here's an update with the typos fixed, the copyright's updated, and the test included.
Created attachment 164855 [details] Updates to the patches I think everything is included this time.
FYI: I've opened a CQ to review this contribution and am waiting for the outcome.
(In reply to comment #16) > FYI: I've opened a CQ to review this contribution and am waiting for the > outcome. I have requested TIBCO to submit an Employer Consent Form to cover this and any other future contributions. Hopefully this will not take too long...
From the better-late-than-never department: I finally took a good look at the patch, and it looks great to me. Apologies for the very long delay; I've had too many things languishing on my todo list. At least, by the looks of things, I'm not slower than legal. ;-) One thing I did notice is that, in the latest patch, there are still synchronized statements in ComposedSwitch.addSwitch(), removeSwitch() and findDelegate(), even though it appears the decision in the comments above was to remove them (and rebuild the whole map when a delegate needs to be added in findDelegate()). I agree with that change, but it appears to have gotten lost somewhere along the way.
(In reply to comment #16) > FYI: I've opened a CQ to review this contribution and am waiting for the > outcome. I have today submitted TIBCO's Employer Consent Form to Barb Cochrane/Sharon Corbett. Do I need to do anything further on the legal/admin front? Thanks, --A
Adrian, I think that's it then. Thanks!
The CQ https://dev.eclipse.org/ipzilla/show_bug.cgi?id=3937 is approved. Unfortunately we're significantly past API freeze so we'll tackle this first thing next release.
(In reply to comment #21) > The CQ https://dev.eclipse.org/ipzilla/show_bug.cgi?id=3937 is approved. > Unfortunately we're significantly past API freeze so we'll tackle this first > thing next release. Okay Ed, no worries, I was kinda prepared for that. Apologies for the recent radio silence - TIBCO's new flagship AMX-BPM MDD platform has just gone GA so have been preoccupied of late. Regarding the synchronized blocks - would you like me to do the patch? Cheers, --A
It should be an easy change. I'm concerned if you contribute further, I'd need to update the CQ, so better I do these details.
The fix is committed to CVS for 2.7. Better late than never. Thanks for the contribution!
The changes are available in EMF 2.7 M7 or an earlier build.
