Re: target-delegates.c needs some TLC [was Re: [OB PATCH] target.h (to_traceframe_info): Fix TARGET_DEFAULT_RETURN]

[Doug Evans <xdje42 at gmail dot com>]

On Fri, Mar 7, 2014 at 10:33 AM, Pedro Alves <palves@redhat.com> wrote:
> On 03/06/2014 05:20 AM, Doug Evans wrote:
>> On Mon, Mar 3, 2014 at 5:36 PM, Yao Qi <yao@codesourcery.com> wrote:
>>> On 03/04/2014 09:18 AM, Hui Zhu wrote:
>>>> I cannot understand about this OB is not right.  I have 2 questions to you:
>>>> 1. Before my patch, does target-delegates.c that generated by make-target-delegates is same with current target-delegates.c?
>>>
>>> No, as I said, I forgot to re-generate target-delegates.c.
>>
>> Hmmm....
>>
>> I don't even see target-delegates.c in Makefile.in.  That feels like a
>> bug. [Could be blind of course. :-)]
>> I realize there's a comment in target-delegates.c that says how to
>> regenerate it, but these kind of things are part of what makefiles are
>> for.
>
> This also crossed my mind when initially reviewing the series (and
> I'm sure Tom's too when writing it, as it's such an obvious thing),
> but realized this is really no different from e.g., gdbarch.h|c.
>
> So given the precedent, I don't consider this a particular bug of
> target-delegates.c, but a more generic "IWBN if we had Makefile
> rules for our generated files".

It wouldn't be gdb if finding one bug didn't expose one or several more. :-)

In this case, these kinds of things are trivial to write, IMO they
should be part of the patch.
It's s.o.p.

> Of course, I'd welcome patches in that direction.

I'd say let's not make the problem worse.
This is trivial stuff, and someone has already tripped over this.
[--enable-maintainer-mode would likely *not* have prevented it the
first time for any individual (they're likely not aware of
--enable-maintainer-mode), but we *should* be providing the means for
this to automagically work]

>> I'm not sure I'd want to require perl for --enable-maintainer-mode
>> (which is a common trigger for enabling in makefiles the appropriate
>> rules to auto-regenerate checked-in machine-generated files), but it's
>> one thought.
>
> I don't see a problem there.  automake is perl as well, for instance,
> and it's common for --enable-maintainer-mode to trigger automake/aclocal.

I recognize the *technical* need of having perl installed for use by
automake, but that's not the context in which my comment is phrased.
How much perl do I need to know in order to use automake?
[If perl isn't installed and I do "yum install automake", the
necessary bits of perl get installed (I hope) but Joe Developer
needn't be aware of that.]
My comment is phrased the way it is because I was not prepared to
a-priori declare one needs to know perl when --enable-maintainer-mode
is turned on: If the user turns it on and that exposes a
problem/issue/whatever with target-delegates.c, then I expect the user
to have to deal with it, it's one more piece if implementation
infrastructure we've imposed on developers.  Today, if a developer
turns on --enable-maintainer-mode I'm not going to apologize for
having imposed on them some minimal comfortability with autoconf.
Typically one needn't know very much (hopefully just having the right
version installed will solve their problem), but IME that's not always
the case.
Help is always at hand of course, still there's a non-trivial
additional load being imposed on developers, and I'm not prepared to
a-priori impose it.

btw, for reference sake,
I can imagine another reason for wanting something other than
--enable-maintainer-mode:
Depending on what files one is expecting to be editing (configure.ac
or target.h or ...?) one might not have the right versions of the
tools installed in order to avoid spurious difference in the generated
files, and I can imagine IWBN if I was hacking on one file to not want
to have to make sure I have the right versions of all the tools that
--enable-maintainer-mode uses.
This is more IWBN though, the converse of course is a growing number
of --enable-foo options which has its own problems.

> Even if that weren't true, by configuring with --enable-maintainer-mode,
> by definition you're asserting you have the tools required for
> regular gdb maintenance, and given make-target-delegates is perl,
> well, any maintainer must have it handy.