[PATCH v1 1/2][PPC64] [PR88877]

[Segher Boessenkool]

On Tue, Jun 09, 2020 at 02:29:13PM -0600, Jeff Law wrote:
> On Sun, 2020-05-24 at 11:22 -0500, Segher Boessenkool wrote:
> > OTOH, you don't need to name Tuple at all...  It should not *have* a
> > constructor, since you declared it as class...  But you can just use
> > std::tuple here?
> > 
> > >         (emit_library_call): Added default arg unsigned_p.
> > >         (emit_library_call_value): Added default arg unsigned_p.
> > 
> > Yeah, eww.  Default arguments have all the problems you had before,
> > except now it is hidden and much more surprising.
> > 
> > Those functions really should take rtx_mode_t arguments?
> > 
> > Thanks again for working on this,
> ISTM that using std::tuple would be better than defining our own types.

Yeah.  But as Jakub an Iain said, not using a container type (but a more
concrete type, instead) is much better anyway :-)

> I'd rather see the argument be explicit rather than using default arguments too. 
> While I have ack'd some patches with default arguments, I still don't like 'em.

Default arguments have their place (but it's not here :-) )

> I do like the approach of getting the infrastructure in place without changing
> behavior, then having the behavior fix as a distinct change.

With Git, commits are easy and cheap, and massaging a patch series into
shape is easy and cheap as well.  If you develop using Git in the first
place (and you should!), you should naturally end up with many patches
in your series, and the preparatory patches first (after you reshuffle
things a bit, if you are like me and your foresight is severly limited).

So you have this separate *anyway* (or should have).  Since it helps
reviewing a lot, and also later bisecting, it is good to keep it.


Segher