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

[Pedro Alves <palves@redhat.com>]

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.

Thanks,
Pedro Alves