Bugzilla – Full Text Bug Listing
| Summary: | [commands] Add an ICommandControlService interface. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Pawel Piech <pawel.1.piech> | ||||||
| Component: | cdt-debug-dsf | Assignee: | Marc Khouzam <marc.khouzam> | ||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | cdtdoug, dd.general-inbox, marc.khouzam | ||||||
| Version: | 0 DD 1.1 | Flags: | cdtdoug:
iplog-
|
||||||
| Target Milestone: | DD 1.1 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Pawel Piech
Very elegant. I like it a lot. +1 Created attachment 109546 [details]
Updated patch.
I added one more method to the new interface and committed.
Changes committed, Marc please verify. (In reply to bug 240507 comment #7) > I have one concern. > Unlike other services, the ICommandControlService, can only deal with a single > context. This means that we'll need multiple instances of that service, one > per commandControl. I'm a little worried about registering with OSGI and > service tracking for such multiple instances. > > I won't mark as verified until you confirm that was indeed your intention. Yes this was indeed my intention. If there are going to be multiple back ends active, they will be distinguished using their IDs. The logic to do this could be somewhat ugly, but it could be hidden using a command control proxy that we talked about before. > Some minor comments: > o In the constructor of GDBControlInitializedDMEvent and > GDBControlShutdownDMEvent, it may be better to have the type of the parameter > to be ICommandControlDMContext instead of GDBControlDMContext. This would give > more flexibility in case someone wants to create a new context, without having > to change those constructors. Good point, I'll change this. > o Should we @Deprecate AbstractMIControl.getControlDMContext() and > GDBControl.getControlDMContext()? Sure, why not :-) > o In GDBControl.RegisterStep, should we register as an ICommandControlService > also? Yes! Thank you. > o In MIControlDMContext, do we accept the commandControlId to be null? If so, > we'll get an exception in equals() and hashCode(). Correct, the ID cannot be null. But since it's just a string I don't think it'll be a problem. Besides it's been like this in 1.0 as well. As I'm debugging with GDB 6.8, I'm seeing that termination processing is broken. I don't know if it was my changes or yours that broke it, but I'll try to look into it a bit. (In reply to comment #4) > (In reply to bug 240507 comment #7) > > I have one concern. > > Unlike other services, the ICommandControlService, can only deal with a single > > context. This means that we'll need multiple instances of that service, one > > per commandControl. I'm a little worried about registering with OSGI and > > service tracking for such multiple instances. > > > > I won't mark as verified until you confirm that was indeed your intention. > Yes this was indeed my intention. If there are going to be multiple back ends > active, they will be distinguished using their IDs. The logic to do this could > be somewhat ugly, but it could be hidden using a command control proxy that we > talked about before. Ok, I see. I guess the commandProxy would register with OSGI, but not the different ICommandControlServices. And the CommandProxy would instantiate the commandControlServices itself. That doesn't sound that bad. So, verified :-) I'll look into your findings for the process termination thing. (In reply to comment #5) > I'll look into your findings for the process termination thing. It looks like a bug in MIBreakpointManager (see bug 24050 comment #8), and I'm guessing its related somehow to the multi-process changes. (In reply to comment #4) > > o In the constructor of GDBControlInitializedDMEvent and > > GDBControlShutdownDMEvent, it may be better to have the type of the parameter > > to be ICommandControlDMContext instead of GDBControlDMContext. This would give > > more flexibility in case someone wants to create a new context, without having > > to change those constructors. > Good point, I'll change this. Did you decide against this or did it slip between the cracks? (In reply to comment #7) > Did you decide against this or did it slip between the cracks? It slipped, thanks for reminding me. DD 1.1 reelased! |