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

Bug 337303

Summary: Refactor DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget
Product: [Tools] CDT Reporter: Marc Khouzam <marc.khouzam>
Component: cdt-debug-dsfAssignee: Nobody - feel free to take it <nobody>
Status: RESOLVED FIXED QA Contact: Pawel Piech <pawel.1.piech>
Severity: minor    
Priority: P3 CC: aleherb+eclipse, cdtdoug, nobody
Version: 8.0Flags: marc.khouzam: review+
Target Milestone: 8.0   
Hardware: PC   
OS: Linux   
Whiteboard:
Attachments:
Description Flags
Refactoring of DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget.
nobody: iplog-
Updated patch.
nobody: iplog-
Added getBreakpointType() to DisassemblyToggleTracepointsTarget.
nobody: iplog-
Modified patch. nobody: iplog-

Description Marc Khouzam CLA 2011-02-16 06:46:45 EST 
Like it was done for Bug 336875, we can also refactor DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget
which are almost identical.  Those two classes are in the dsf.ui and dsf.gdb.ui plugins respectively because they use the code from the DSF Disassembly view.  I think putting the abstract class in dsf.ui and extending it in both dsf.ui and dsf.gdb.ui would be good.
Comment 1 Nobody - feel free to take it CLA 2011-02-17 22:29:18 EST 
Created attachment 189247 [details]
Refactoring of DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget.
Comment 2 Nobody - feel free to take it CLA 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 CLA 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 CLA 2011-02-18 11:43:55 EST 
Created attachment 189294 [details]
Updated patch.
Comment 5 Marc Khouzam CLA 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 CLA 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 CLA 2011-02-18 12:00:09 EST 
Created attachment 189300 [details]
Added getBreakpointType() to DisassemblyToggleTracepointsTarget.
Comment 8 Marc Khouzam CLA 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 CLA 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 CLA 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 CLA 2011-02-22 13:46:39 EST 
I guess I can commit this patch.
Comment 12 Nobody - feel free to take it CLA 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 CLA 2011-02-22 13:55:45 EST 
Thanks Mikhail