Re: [PATCH/RFC] Refactor gdb.reverse/insn-reverse.c

[Luis Machado <lgustavo@codesourcery.com>]

On 01/25/2017 11:44 AM, Pedro Alves wrote:
> On 01/25/2017 04:50 PM, Luis Machado wrote:
>> On 01/25/2017 10:10 AM, Yao Qi wrote:
>>> On 17-01-16 13:10:54, Luis Machado wrote:
>>>> I've broken up the main file into other files with arch-specific bits
>>>> (insn-support-<arch>.c). The main file will hold the generic pieces
>>>> that will
>>>> take care of calling the tests.
>>>>
>>>
>>> It is reasonable to me.  Can we name arch-specific files as
>>> insn-reverse-<arch>.c?
>>>
>>
>> Thanks for the review.
>>
>> Would you reconsider this? I named it insn-support-<arch>.c because we
>> already know this is a reverse debugging test (from gdb.reverse) and we
>> are really testing instruction support. I'm fine either way though, and
>> just wanted to add a little bit more context in the name.
>
> My 2c nit.
>
> "support" doesn't sound ideal to me.  By that logic,
> every test in the testsuite is testing gdb's support of some
> feature, so "support" is redundant?
>
> When I see a file named "support", I think it's some base/foundation
> (==support) framework.  Is that the case here?  I had understood
> that the "base" is in insn-reverse.c instead?  Or are the
> insn-support-<arch>.c files meant to be shared between multiple
> test cases, thus they'll be providing "support" for a multitude
> of random tests?  insn-reverse-<arch>.c at least gave a hint
> that the files are all related to insn-reverse.c.
>
> Also, if "reverse" is redundant in "insn-reverse-<arch>.c", then
> it is also redundant in "insn-reverse.c" too, so you should be arguing
> for renaming "insn-reverse.c|exp" -> "insn-support.c|exp" too, which
> would preserve the similarity of the names between the driver
> file + the arch files.

That is a reasonable assessment. insn-reverse.[c|exp] is redundant and 
IMO would benefit from renaming too.

The support in "insn-support-<arch>.c means support for a set of 
instructions for this particular subsystem of gdb, therefore why i went 
with that name. Thinking about it further, instruction decoding support 
is the basis/foundation of reverse debugging, without which things would 
not work properly. But i may be overthinking. :-)

I didn't analyze it in depth though. It just seemed to me the naming was 
slightly better with less redundancy.

But, like i said, i'm fine either way.