Bugzilla – Full Text Bug Listing
| Summary: | [services] Service factory should be used to create ICommandControl | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Marc Khouzam <marc.khouzam> | ||||||||||
| Component: | cdt-debug-dsf | Assignee: | Pawel Piech <pawel.1.piech> | ||||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | cdtdoug, dd.general-inbox, pawel.1.piech | ||||||||||
| Version: | 0 DD 1.1 | Flags: | cdtdoug:
iplog-
|
||||||||||
| Target Milestone: | DD 1.1 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Marc Khouzam
Created attachment 108369 [details]
First part of fix
This patch removes the abstract from the methods, and adds ICommandControl, ISignals and ISymbols. It also uses alphabetical order to make it easier to know if something is forgotten.
Committed.
Created attachment 108506 [details]
Ugly solution that works
I now remember why I hadn't put the ICommandControl service as part of the service factory. It was because the constructor to GDBControl took a bunch of parameters, unlike all the other services.
This solution makes the GDBControl constructor the same as all the other ones, and add set...() methods for all the other missing parameters. I don't like it much.
Any better suggestions?
Also, I left the old constructor and deprecated it. Then I realized this was a provisional class, so I'm thinking I can remove that constructor completely, yes?
Committed (but if a better solution is suggested, I'll change it with pleasure)
(In reply to comment #2) Pawel wrote this comment in another bug by mistake: "That's indeed rather ugly :-(. Another option may be to just pass the launch configuration in a single set...() call, then let GDBControl extract the parameters as needed. This may create some code duplication, but maybe not." Created attachment 108679 [details] Cleanup of GDBControl constructor (In reply to comment #3) > (In reply to comment #2) > Pawel wrote this comment in another bug by mistake: > "That's indeed rather ugly :-(. Another option may be to just pass the launch > configuration in a single set...() call, then let GDBControl extract the > parameters as needed. This may create some code duplication, but maybe not." Actually, it turned out to be quite clean. Thanks for the idea. I had another idea. I'll attach a patch to illustrate. Created attachment 108683 [details]
Use of variable number of arguments
The service factory only allows to pass a single parameter to the service when creating it. This patch adds a variable list of parameters to the signature of createService(). It allows any number of parameters to be passed to a service. This can prove useful for future service versions.
The problem is that you loose some compile-time checks on the parameters. However, this is relatively isolated, so I don't think it is a risk.
Opinions?
(In reply to comment #5) > Opinions? I don't see how it would cause any harm, so +1 from me. (In reply to comment #6) > (In reply to comment #5) > > Opinions? > I don't see how it would cause any harm, so +1 from me. Great. I re-ordered the parameters to have the name of the class first and then all parameters, including the session and I committed. Pawel, can you verify? Reviewed. DD 1.1 reelased! |