Comment 1 Nobody - feel free to take it 2011-02-17 22:29:18 EST

Created attachment 189247 [details]
Refactoring of DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget.

Comment 2 Nobody - feel free to take it 2011-02-17 22:34:09 EST

Toni, please take a look at the patch. I put the abstract class in the provisional package for disassembly. Is that the right place for it?

Comment 3 Anton Leherbauer 2011-02-18 03:05:33 EST

Looks good, just a few remarks:

- The method
      protected int getBreakpointType()
  in DisassemblyToggleBreakpointsTarget seems superfluous.

- The abstract class could also become public API instead of provisional, 
  because it is an abstract implementation of a public interface and does not 
  expose any provisional or internal API.

Comment 4 Nobody - feel free to take it 2011-02-18 11:43:55 EST

Created attachment 189294 [details]
Updated patch.

Comment 5 Marc Khouzam 2011-02-18 11:53:30 EST

Mikhail, could you use
  protected int getBreakpointType()
for DisassemblyToggleTracepointsTarget as well?

There is a concept of 'fast' tracepoint in GDB which will require to use the HARDWARE type instead of REGULAR.  Also, it would match the refactoring you did in the ToggleTracepointAdapter class.

Comment 6 Nobody - feel free to take it 2011-02-18 11:55:40 EST

(In reply to comment #3)
> Looks good, just a few remarks:
> 
> - The method
>       protected int getBreakpointType()
>   in DisassemblyToggleBreakpointsTarget seems superfluous.
>

Sorry, I have updated the patch. The purpose of this method is to allow subclassing of DisassemblyToggleBreakpointsTarget to support hardware breakpoints (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=332993). Maybe I should just add a constructor that sets the breakpoint type. What do you think?

> - The abstract class could also become public API instead of provisional, 
>   because it is an abstract implementation of a public interface and does not 
>   expose any provisional or internal API.

In this case, is 'org.eclipse.cdt.dsf.ui.actions' package the right place for it?
What is the next version of DSF? I put 2.2 in the since tag but not sure if that's correct.

Comment 7 Nobody - feel free to take it 2011-02-18 12:00:09 EST

Created attachment 189300 [details]
Added getBreakpointType() to DisassemblyToggleTracepointsTarget.

Comment 8 Marc Khouzam 2011-02-18 12:01:49 EST

(In reply to comment #7)
> Created attachment 189300 [details]
> Added getBreakpointType() to DisassemblyToggleTracepointsTarget.

Thanks!

Comment 9 Anton Leherbauer 2011-02-21 04:18:13 EST

(In reply to comment #6)
> Maybe I
> should just add a constructor that sets the breakpoint type. What do you think?

I don't have a strong preference.  An abstract method provides more flexibility if that's ever desired.

> In this case, is 'org.eclipse.cdt.dsf.ui.actions' package the right place for
> it?

Sounds suitable.  It's definitely action-related.

> What is the next version of DSF? I put 2.2 in the since tag but not sure if
> that's correct.

Correct. API Tooling should give you a hint if it's wrong.

Comment 10 Nobody - feel free to take it 2011-02-21 14:05:19 EST

Created attachment 189435 [details]
Modified patch.

Moved AbstractDisassemblyBreakpointsTarget to org.eclipse.cdt.dsf.debug.ui.actions.

Comment 11 Nobody - feel free to take it 2011-02-22 13:46:39 EST

I guess I can commit this patch.

Comment 12 Nobody - feel free to take it 2011-02-22 13:53:09 EST

Committed to HEAD.
Marc, please review. Again, it's just a formality - Toni and you have already reviewed it.

Comment 13 Marc Khouzam 2011-02-22 13:55:45 EST

Thanks Mikhail

Comment 14 CDT Genie 2011-02-22 14:23:25 EST

*** cdt cvs genie on behalf of mkhodjai ***
Bug 337303: Refactor DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget

[*] DisassemblyToggleBreakpointsTarget.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/DisassemblyToggleBreakpointsTarget.java?root=Tools_Project&r1=1.1&r2=1.2

[+] AbstractDisassemblyBreakpointsTarget.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/ui/actions/AbstractDisassemblyBreakpointsTarget.java?root=Tools_Project&revision=1.1&view=markup

[*] DisassemblyToggleTracepointsTarget.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/breakpoints/DisassemblyToggleTracepointsTarget.java?root=Tools_Project&r1=1.3&r2=1.4