Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 325676

Summary: [disassembly] add dsf/debug/internal/provisional/service/IInstructionWithSize.java for 7.0.2
Product: [Tools] CDT Reporter: Kirk Beitz <kirk.beitz>
Component: cdt-debug-dsfAssignee: Anton Leherbauer <aleherb+eclipse>
Status: RESOLVED FIXED QA Contact: Pawel Piech <pawel.1.piech>
Severity: normal    
Priority: P3 CC: aleherb+eclipse, ken.ryall, pawel.1.piech
Version: 7.0.1   
Target Milestone: 7.0.2   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
new IInstructionWithSize interface, DisassemblyBackendDsf use of new interface, EDCInstruction implementation of new interface, all for commit ONLY to cvs branch to be used for CDT 7.0.2 aleherb+eclipse: iplog+, aleherb+eclipse: review+

Description Kirk Beitz CLA 2010-09-18 09:04:59 EDT 
Created attachment 179170 [details]
new IInstructionWithSize interface, DisassemblyBackendDsf use of new interface, EDCInstruction implementation of new interface, all for commit ONLY to cvs branch to be used for CDT 7.0.2

this bug is being split primarily for maintenance/tracking purposes.

Bug 325277 was originally created to add new function IInstruction#getSize() before i knew the API change would not be allowed.

the attached patch is a proposal that will add an internal "provisional" service interface IInstructionWithSize with only the function getSize() that can be committed to eclipse cvs for use in the eventual cdt 7.0.2 release.

the patch also contains the DisassemblyBackendDsf#insertDisassembly() mods that check for instanceof IInstructionWithSize and call the new getSize() when available.  the patch also has the companion DisassemblyDocument.java mods to limit fMeanInstructionSize.  for completeness, the patch includes EDCInstruction.java and its implementation of InstructionWithSize#getSize(), but it is not necessary to commit it together with the rest of the patch for it to build and run.

if accepted, this patch can be applied/committed completely independently of the patch for Bug 325277.  However, because Bug 325284 (which is release branch independent) is indirectly dependent upon both that bug and this one for the same reason (namely, because it fills EDCInstruction and then depends upon it telling DisassemblyBackendDsf#insertDisassembly() the proper size of its instructions in order to avoid a runtime infinite loop) … it would probably be best to accept both together … even if they are committed at slightly different times and/or closed at slightly different times due to the requirements of release cycles.
Comment 1 Anton Leherbauer CLA 2010-09-28 08:31:27 EDT 
I have committed the patch to cdt_7_0 with a few changes:
- plugin version incremented to 2.1.1 (instead of 2.2)
- marked new provisional package as internal

I omitted the EDC part.

The patch included also the changes for bug 324507, which I will update accordingly.
Comment 2 Kirk Beitz CLA 2010-09-30 16:46:10 EDT 
(In reply to comment #1)
> I have committed the patch to cdt_7_0 with a few changes:
> - plugin version incremented to 2.1.1 (instead of 2.2)
> - marked new provisional package as internal
> 
> I omitted the EDC part.
> 
> The patch included also the changes for bug 324507, which I will update
> accordingly.

given that marking the provisional package internal makes dealing with the EDC part that much more of a pain (the only fix is apparently adding API filters) …

and given that, for the change you committed to HEAD for using AbstractInstruction … you stated:

> API breakage is avoided using an extension interface (IInstructionWithSize)
> similar to the solution on the maintenance branch, but this time as public API.

would it be possible to commit this same set of changes to cdt_7_0 for 7.0.2 as well in place of what's been committed thus far?  that would also put cdt_7_0 and HEAD in lock-step with regard to this change: good for anyone other than EDC who wants to have an IInstruction/IInstructionWithSize/AbstractInstruction solution that is supported on both branches.

at the very least, you'd be helping us out:  it would certainly be by far cleaner for us to be able to simply mirror the branch without special problem-filters for things to build.

thanks.
Comment 3 Anton Leherbauer CLA 2010-10-01 02:42:37 EDT 
Sorry, API additions would also be flagged as a problem by API tooling.  A provisional interface is the appropriate approach here.  We can add the relevant EDC plug-in as a "friend", if you want to get rid of the warning.
Comment 4 Kirk Beitz CLA 2010-10-01 03:29:24 EDT 
(In reply to comment #3)
> Sorry, API additions would also be flagged as a problem by API tooling.  A
> provisional interface is the appropriate approach here.  We can add the
> relevant EDC plug-in as a "friend", if you want to get rid of the warning.

yeah, i should have realized this was going to happen; i was probably blinded by the glimmer of hope of having the same code on both branches.

adding ";x-friends="org.eclipse.cdt.debug.edc" to MANIFEST.MF would help; from a warning standpoint, we were designing this this way, so the warning seems like noise.  and having the "x-friends" marker in the MANIFEST would make it clear what the provisional interface was intended for.

thanks.
Comment 5 Anton Leherbauer CLA 2010-10-01 05:09:17 EDT 
I added "org.eclipse.cdt.debug.edc" as friend, but I got a new error about "EDCInstruction implements a non-API interface".  I guess the only workaround is to filter this problem.
Comment 6 Kirk Beitz CLA 2010-10-01 10:36:38 EDT 
(In reply to comment #5)
> I added "org.eclipse.cdt.debug.edc" as friend, but I got a new error about
> "EDCInstruction implements a non-API interface".  I guess the only workaround
> is to filter this problem.

yes, understood.  my plan EDCInstruction.java was actually to change the API Errors/Warnings for just this one plug-in to leave that particular errror as a warning, rather than the api filter.  i'll run that by ken, and we'll figure out what seems best.

i still think the friend tag in the manifest is appropriate in terms of intent & documentation, so thanks for doing that.