Re: [PATCH] Improve readability of x86's SystemTap SDT probe support & fix bug & add testcase

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Saturday, December 28 2013, Joel Brobecker wrote:

>> This patch reorganizes code on gdb/i386-tdep.c's SystemTap SDT probe
>> support functions.  Before it, the code to parse special operands on x86
>> lived in a single, big function.  This patch creates 2 new functions
>> that makes the code more organized and removes a few indentation levels
>> (which is always good IMO).
>> 
>> While I was making the patch, I decided to create some testcases for it
>> (by tweaking the probe's asm generated by GCC), and found a bug on the
>> parsing code for the triplet displacement operand (i.e., things like
>> "-4+8-20(%rbp)").  Well, since I was already at it, I created some
>> testcases for the two kinds of special operands handled by the code (the
>> triplet displacement mentioned above, and the three-argument
>> displacement like "(%rbx,%ebx,-8)").  This code is target-dependent so I
>> created it under gdb.arch/.
>> 
>> Comments?  OK to apply?
>
> First of all, thanks for creating a testcase!

My pleasure :-).

> I've only skimmed through the patch so far. Generally speaking,
> I definitely agree that simplifying large functions is a useful
> thing to do. Fixing bugs too :). It would be better, though, if
> we could separate the two into different patches, one that does
> the re-org, and nothing else. This one can be large, but we know
> the code hasn't really be changed. Perhaps "git diff/show -b"
> will even confirm that. And the second can include the fix and
> the testcase, and it'll be obvious from the diff what the fix is.
>
> Would you mind doing that?

Absolutely!  I will post the two patches soon :-).

Thanks!

-- 
Sergio