This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]