Review Request: Memory viewer fixes.

[Ben Wagner]

> On April 13, 2012, 5:08 p.m., Friedrich W. H. Kossebau wrote:
> > Never seen this code before, so can only comment from how the KHexEdit interface is used on the quick look I gave. Nothing really bad seen here :)

The one bad thing I've since noticed was my use of stdint.h for uintptr_t. It seems quintptr is more correct.


> On April 13, 2012, 5:08 p.m., Friedrich W. H. Kossebau wrote:
> > debuggers/gdb/memviewdlg.cpp, lines 187-195
> > <http://git.reviewboard.kde.org/r/104574/diff/1/?file=56511#file56511line187>
> >
> >     In general I wonder if it might make sense to switch from using the KHexEdit interfaces and a run-time dependency on effectively the Okteta libs to using the Okteta libs directly and a build-time dependency on them.
> >     After all there is already a build-time dependency on the Okteta libs, for the Okteta plugin to allow raw-editing of files.
> >     
> >     Having direct access to the richer interfaces of the Okteta libs could perhaps enable things that might have been blocked before.
> >     
> >     See http://api.kde.org/4.8-api/kdesdk-apidocs/okteta/html/namespaceOkteta.html (and ignore all the classes in the namespace Kasten* , only useful if using the Kasten framework. Okteta:: is yours for just the Okteta libs). You would ideally subclass the Okteta::AbstractByteArrayModel to provide a model of the memory you want to display/edit. Or just reuse of one the existing and stuff it with the data.
> >     
> >     Just food for thoughts for future development :)

I did not realize there was already a dependency on Okteta for other reasons. Using it directly would make a lot more sense. There are a number of limitations with the KHexEdit interface. There were some nasty hacks I removed which were originally intended to get around these limitations. I'll take a look into using it directly.


> On April 13, 2012, 5:08 p.m., Friedrich W. H. Kossebau wrote:
> > debuggers/gdb/memviewdlg.cpp, lines 228-229
> > <http://git.reviewboard.kde.org/r/104574/diff/1/?file=56511#file56511line228>
> >
> >     These two lines are contradicting.
> >     
> >     It should be decided to either set the widget to a fixed number of bytes per line or to have the widget put as many bytes on a line as possible with the current widget width.

Yes, they are. This was just more or less fixing up (i.e. making work) what the previous code did. A number of these settings should be user settable or preferences.


> On April 13, 2012, 5:08 p.m., Friedrich W. H. Kossebau wrote:
> > debuggers/gdb/memviewdlg.cpp, line 338
> > <http://git.reviewboard.kde.org/r/104574/diff/1/?file=56511#file56511line338>
> >
> >     Could get a comment what this line tries to do, i.e. to prevent access to the data that is deleted on the next line.
> >     Because on a first look I was puzzled that after the next line this->data_ is no longer existing :)

I was getting glibc memory corruption errors, and this seemed to fix it. The idea is, as you guessed, to detatch the data from the editor before updating. It would probably make more sense to create the new data, swap, then delete the old data.


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104574/#review12402
-----------------------------------------------------------


On April 13, 2012, 12:19 a.m., Ben Wagner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104574/
> -----------------------------------------------------------
> 
> (Updated April 13, 2012, 12:19 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> This is a somewhat odd patch, since it changes what is currently dead code. However, uncommenting the relevant portions of debuggerplugin.* and CMakeLists in debuggers/gdb allows it to be used. This change makes the memory viewer at least work and not crash, which is an improvement over the existing state. This code needs more work before being re-enabled, as does the code which calls it (it is rather strange the way it's UI is set up). However, this is a good checkpoint.
> 
> 
> Diffs
> -----
> 
>   debuggers/gdb/memviewdlg.h 1629cd0 
>   debuggers/gdb/memviewdlg.cpp 0500a21 
> 
> Diff: http://git.reviewboard.kde.org/r/104574/diff/
> 
> 
> Testing
> -------
> 
> Got it working. Used it. When Okteta is installed it works and I haven't gotten it to crash again yet.
> 
> 
> Thanks,
> 
> Ben Wagner
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120413/bf7c96f1/attachment.html>