Review Request 121456: GDB plugin enhancements and some command queue optimizations

[Nicolai Hähnle]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121456/#review71848
-----------------------------------------------------------


Hey Andreas, thank you for putting this up here, and see some comments below.

As a general question, do you think you will be able to port these changes to the current, KF5-based master branch? Most of your changes should apply as they are, the major difference at this point is the use of kDebug vs. qCDebug.


debuggers/gdb/debugsession.cpp
<https://git.reviewboard.kde.org/r/121456/#comment50065>

    I feel like this should be done in GDBCommandQueue::enqueue.
    
    This way, the command queue is responsible for this part of maintaining invariants, which makes the code more robust in case somebody decides to call enqueue() elswhere in the future.



debuggers/gdb/disassemblewidget.h
<https://git.reviewboard.kde.org/r/121456/#comment50062>

    As per the interface design recommendations, I believe it would be better to have an enum for the disassembly flavor.
    
    (Consider looking at a call-site of this method some months down the road: Would you rather see window->setDisassemblyFlavor(true); or window->setDisassemblyFlavor(FlavorAtt);?)



debuggers/gdb/disassemblewidget.cpp
<https://git.reviewboard.kde.org/r/121456/#comment50063>

    Is addCommandToFront really necessary here? I think it would be best to use addCommand wherever possible.
    
    In general, less magic in the order of command processing is better.



debuggers/gdb/disassemblewidget.cpp
<https://git.reviewboard.kde.org/r/121456/#comment50064>

    Same here about addCommand vs. addCommandToFront



debuggers/gdb/gdbcommand.h
<https://git.reviewboard.kde.org/r/121456/#comment50066>

    I'm unhappy about not being able to tell the units from these variable names. Perhaps rename them to XxxxMsecs?



debuggers/gdb/gdbcommandqueue.cpp
<https://git.reviewboard.kde.org/r/121456/#comment50067>

    We definitely have a problem with duplicate commands being issued unnecessarily. However, I have to admit that I don't think this is really the right solution.
    
    The thing is, commands also have callbacks, and at this point of the code, we simply have no knowledge of whether removing the command messes up assumptions made by the code that issued the command.
    
    I think we should fix the problem at the source, i.e. going through the various subsystems that issue the duplicate commands and making them smarter about when to update.
    
    Finally, this code leaks memory; the command list stores pointers, which have to be deleted explicitly.



debuggers/gdb/gdbcommandqueue.cpp
<https://git.reviewboard.kde.org/r/121456/#comment50068>

    Same remarks (including leaking memory) apply here.


- Nicolai Hähnle


On Dez. 12, 2014, 6:06 vorm., Andreas Roth wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121456/
> -----------------------------------------------------------
> 
> (Updated Dez. 12, 2014, 6:06 vorm.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> Repo is available at https://github.com/aroth-arsoft/kdevelop/tree/4.7-gdb-enhancements
> - switch the assembler flavor from AT&T to Intel and back in the dissassembler widget
> - remove StackList commands from the queue when a new Exec command arrives. This reduces the wait time when stepping through the code quickly.
> - remove duplicated command in the queue. Mostly to eliminate duplicate variable updates.
> - show the pending command in the command queue. This is purely for debugging and to check which commands are currently pending.
> - slight improvements to memory view to use sizeof() when no size argument is given
> 
> 
> Diffs
> -----
> 
>   debuggers/gdb/debugsession.cpp edc0d34473e99230db510457fd609a178bc8fa34 
>   debuggers/gdb/disassemblewidget.h ccffc2f0325aac8c8486ff413ed64219548461c3 
>   debuggers/gdb/disassemblewidget.cpp 91194ed7e69ff87ebdfbb5383c1b38939760d098 
>   debuggers/gdb/gdb.cpp 46420876149604a318caec7111f252dcc12b39b4 
>   debuggers/gdb/gdbcommand.h e1ebc6988fb508a187d2775735c13b9123fcda04 
>   debuggers/gdb/gdbcommand.cpp 67abbd87f1f0d10a84a45b3ee9f104c77ffe925b 
>   debuggers/gdb/gdbcommandqueue.h 7d82f45a244d9fffb9b3a2ba5a82eaaa06f5c8d0 
>   debuggers/gdb/gdbcommandqueue.cpp 801bc41e79f62a226de04cd1a91500c86a9cf1a3 
>   debuggers/gdb/memviewdlg.cpp cf9d0b49ea0b617b405a5065354af9c9e59afa8d 
> 
> Diff: https://git.reviewboard.kde.org/r/121456/diff/
> 
> 
> Testing
> -------
> 
> Only preliminary testing done; haven't checked the unit-tests yets
> 
> 
> Thanks,
> 
> Andreas Roth
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20141212/2f54d68c/attachment-0001.html>