Bugzilla – Full Text Bug Listing
| Summary: | Refactor ToggleBreakpointAdapter and ToggleTracepointAdapter classes | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Nobody - feel free to take it <nobody> | ||||||||
| Component: | cdt-debug | Assignee: | Nobody - feel free to take it <nobody> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | Ken Ryall <ken.ryall> | ||||||||
| Severity: | enhancement | ||||||||||
| Priority: | P3 | CC: | cdtdoug, marc.khouzam, pawel.1.piech | ||||||||
| Version: | 8.0 | Flags: | nobody:
iplog-
marc.khouzam: review+ |
||||||||
| Target Milestone: | 8.0 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows 7 | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 332993 | ||||||||||
| Attachments: |
|
||||||||||
Nice improvement. It will greatly help for the hardware bp addition (I assume that is your motivation :-)) One question. In the new AbstractToggleBreakpointAdapter, you no longer handle the DisassemblyEditor case. I hadn't put support for it in Tracepoints, but do we want to remove the support for it in breakpoints? I did notice that you still check for DisassemblyEditor in AbstractToggleBreakpointAdapter#canToggleLineBreakpoints() only (In reply to comment #1) > Nice improvement. It will greatly help for the hardware bp addition (I assume > that is your motivation :-)) > I would say that's one of my motivations. > One question. In the new AbstractToggleBreakpointAdapter, you no longer handle > the DisassemblyEditor case. I hadn't put support for it in Tracepoints, but do > we want to remove the support for it in breakpoints? > DisassembltEditor is a remnant of my failed attempt to implement disassembly similar to other debug views. You probably remember that story. > I did notice that you still check for DisassemblyEditor in > AbstractToggleBreakpointAdapter#canToggleLineBreakpoints() only I'll remove it, thanks. (In reply to comment #2) > DisassembltEditor is a remnant of my failed attempt to implement disassembly > similar to other debug views. You probably remember that story. Oh right. I got confused with org.eclipse.cdt.dsf.debug.internal.ui.disassembly.DisassemblyEditor Created attachment 188727 [details]
Removed references to DisassemblyEditor
Created attachment 188729 [details]
Modified patch.
No need to pass the breakpoint type to createLineBreakpoint() and createFunctionBreakpoint().
(In reply to comment #5) > Created attachment 188729 [details] > Modified patch. > > No need to pass the breakpoint type to createLineBreakpoint() and > createFunctionBreakpoint(). Nice, that should make it very easy for the hw bp stuff. It would be worth doing the same refactoring for DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget, which are almost identical. Those two are in the dsf.ui and dsf.gdb.ui plugins 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. (In reply to comment #6) > It would be worth doing the same refactoring for > DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget, > which are almost identical. Those two are in the dsf.ui and dsf.gdb.ui plugins > 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. Marc, this will require a new API because the affected classes are in two different plug-ins. I think it is better to move it to a separate bug. Marc, is it OK to commit this patch? (In reply to comment #7) > (In reply to comment #6) > > It would be worth doing the same refactoring for > > DisassemblyToggleBreakpointsTarget and DisassemblyToggleTracepointsTarget, > > which are almost identical. Those two are in the dsf.ui and dsf.gdb.ui plugins > > 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. > > Marc, this will require a new API because the affected classes are in two > different plug-ins. I think it is better to move it to a separate bug. I opened Bug 337303 about this. (In reply to comment #8) > Marc, is it OK to commit this patch? Of course, sorry for the delay. Committed to HEAD. Marc, asking you for a review is a formality - you have already reviewed the patch :) *** cdt cvs genie on behalf of mkhodjai *** Bug 336875: Refactor ToggleBreakpointAdapter and ToggleTracepointAdapter classes [*] ToggleBreakpointAdapter.java 1.46 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/ToggleBreakpointAdapter.java?root=Tools_Project&r1=1.45&r2=1.46 [*] ToggleTracepointAdapter.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/ToggleTracepointAdapter.java?root=Tools_Project&r1=1.1&r2=1.2 [+] AbstractToggleBreakpointAdapter.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/AbstractToggleBreakpointAdapter.java?root=Tools_Project&revision=1.1&view=markup |
Created attachment 188722 [details] Refactoring of ToggleBreakpointAdapter and ToggleTracepointAdapter. ToggleBreakpointAdapter and ToggleTracepointAdapter mostly share the same code. The attached patch refactors the common parts into a separate abstract class.