Comment 1 Tomas Martinec 2011-03-31 08:10:34 EDT

Created attachment 192266 [details]
Implemented changes

Comment 2 Marc Khouzam 2011-03-31 09:16:01 EDT

Sounds good.  Why do you want to make doInitialize() private?  Sure, it probably should have been, but now that we are past API freeze, we should not change it.

As for getRegistrationClasses(), I like that.  The surprising thing is that this very morning, the same idea surfaced as a potential fix to Bug 341423.  It would create a method like getRegistrationClasses(), but for all services, not just GDBControl.  Let's see where Bug 341423 ends up before doing anything with the current proposed fix.

Comment 3 Tomas Martinec 2011-03-31 16:16:35 EDT

Hello,

> Sounds good.  Why do you want to make doInitialize() private?  Sure, it
> probably should have been, but now that we are past API freeze, we should not
> change it.

I inherited the GDBControl_7_2 and wanted to do some additional
initialization, so I followed the initialize/doInitialize pattern in
this way:

public class MsimControl extends GDBControl_7_2
{
       @Override
   public void initialize(final RequestMonitor requestMonitor) {
       super.initialize( new RequestMonitor(getExecutor(), requestMonitor) {
           @Override
           protected void handleSuccess() {
               doInitialize(requestMonitor);
           }
       });
   }

   private void doInitialize(final RequestMonitor requestMonitor) {
        // Msim specific initialization
        ...
   }
}

However with the doInitialize being public, the GDBControl.initialize
will call MsimControl.doInitialize and the GDBControl.doInitialize
will be skipped. There is easy workaround about this, but I would
guess, that the "Msim specific initialization" was supposed to be done
in my sketched way.

> As for getRegistrationClasses(), I like that.  The surprising thing is that
> this very morning, the same idea surfaced as a potential fix to Bug 341423. It
> would create a method like getRegistrationClasses(), but for all services, not
> just GDBControl.  Let's see where Bug 341423 ends up before doing anything 
> with the current proposed fix.

I have proposed this, because I want to register MsimControl service
and currently, I would have to do something like this:

           register(
               new String[]{ ICommandControl.class.getName(),
                             ICommandControlService.class.getName(),
                             IMICommandControl.class.getName(),
                             AbstractMIControl.class.getName(),
                             IGDBControl.class.getName(),
                             MsimControl.class.getName() },
               new Hashtable<String,String>());

I do not like doing it, because the array of the class names would
have to be updated whenever the base classes structure changes.

Regards,
Tomas

Comment 4 Marc Khouzam 2011-05-04 05:30:18 EDT

(In reply to comment #3)
I've been meaning to get back to this for a while.  Sorry for the delay.

> However with the doInitialize being public, the GDBControl.initialize
> will call MsimControl.doInitialize and the GDBControl.doInitialize
> will be skipped. There is easy workaround about this, but I would
> guess, that the "Msim specific initialization" was supposed to be done
> in my sketched way.

You are absolutely right!  This is pretty bad and should be changed.  The only reason it went unnoticed is that doInitialize was never overridden before.

Thanks for pointing this bug out.

> > As for getRegistrationClasses(), I like that.  The surprising thing is that
> > this very morning, the same idea surfaced as a potential fix to Bug 341423. It
> > would create a method like getRegistrationClasses(), but for all services, not
> > just GDBControl.  Let's see where Bug 341423 ends up before doing anything 
> > with the current proposed fix.
> 
> I have proposed this, because I want to register MsimControl service
> and currently, I would have to do something like this:
> 
>            register(
>                new String[]{ ICommandControl.class.getName(),
>                              ICommandControlService.class.getName(),
>                              IMICommandControl.class.getName(),
>                              AbstractMIControl.class.getName(),
>                              IGDBControl.class.getName(),
>                              MsimControl.class.getName() },
>                new Hashtable<String,String>());
> 
> I do not like doing it, because the array of the class names would
> have to be updated whenever the base classes structure changes.

This is a good idea but will need to wait until the next release, as part of Bug 341660.  See Bug 341660 comment #2 specifically.

Comment 5 Marc Khouzam 2011-05-04 05:41:35 EDT

Created attachment 194688 [details]
Fix committed

This is the patch I committed for the "public doInitialize" problem.  I checked all other instances of doInitialize() and found one more problem so I made the same change in GDBBackend.

Committed to HEAD.

Comment 6 Marc Khouzam 2011-05-04 05:44:17 EDT

I'm marking the bug itself as iplog+ to recognize fyzmat@gmail.com contribution.  I did not mark the patch as iplog+ because only a minor part of the patch was used.

Comment 7 Marc Khouzam 2011-05-04 05:45:29 EDT

Mikhail, can you review this 3-line change?  I may have affected your extension of DSF-GDB.

Comment 8 CDT Genie 2011-05-04 06:23:03 EDT

*** cdt cvs genie on behalf of mkhouzam ***
Bug 341465: doInitialize() must be private to allow each version of the service to get properly initialized.

[*] GDBControl_7_0.java 1.32 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/command/GDBControl_7_0.java?root=Tools_Project&r1=1.31&r2=1.32
[*] GDBControl.java 1.28 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/command/GDBControl.java?root=Tools_Project&r1=1.27&r2=1.28

[*] GDBBackend.java 1.24 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBackend.java?root=Tools_Project&r1=1.23&r2=1.24

Comment 9 Nobody - feel free to take it 2011-05-04 11:50:55 EDT

(In reply to comment #7)
> Mikhail, can you review this 3-line change?  I may have affected your extension
> of DSF-GDB.

Marc, the patch seems to be OK. 
I wasn't following the discussion in https://bugs.eclipse.org/bugs/show_bug.cgi?id=341423 and am getting confused when reading it now. Didn't you, Pawel and John agree on using initialize() + doInitialize() pattern?

Comment 10 Marc Khouzam 2011-05-04 12:52:24 EDT

(In reply to comment #9)
> (In reply to comment #7)
> > Mikhail, can you review this 3-line change?  I may have affected your extension
> > of DSF-GDB.
> 
> Marc, the patch seems to be OK. 
> I wasn't following the discussion in
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=341423 and am getting confused
> when reading it now. Didn't you, Pawel and John agree on using initialize() +
> doInitialize() pattern?

So, we are keeping that pattern for Indigo, and bug 341423 was fixed in an API-compatible way.  For the release after, we'll change the pattern and put some support directly into DSF.  This will be done in Bug 341660.

Thanks for the review