Re: Passing CopyMultiInsertInfo structure to CopyMultiInsertInfoNextFreeSlot()

On Fri, May 17, 2019 at 3:10 AM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> On 2019-May-14, Michael Paquier wrote:
>
> > On Tue, May 14, 2019 at 01:19:30PM +1200, David Rowley wrote:
> > > When I wrote the code I admit that I was probably wearing my
> > > object-orientated programming hat. I had in mind that the whole
> > > function series would have made a good class. Passing the
> > > CopyMultiInsertInfo was sort of the non-OOP equivalent to having
> > > this/Me/self available, as it would be for any instance method of a
> > > class. Back to reality, this isn't OOP, so I was wearing the wrong
> > > hat. I think the unused parameter should likely be removed. It's
> > > probably not doing a great deal of harm since the function is static
> > > inline and the compiler should be producing any code for the unused
> > > param, but for the sake of preventing confusion, it should be removed.
> > > Ashutosh had to ask about it, so it wasn't immediately clear what the
> > > purpose of it was. Since there's none, be gone with it, I say.
> >
> > Sounds fair to me. This has been introduced by 86b8504, so let's see
> > what's Andres take.
>
> If this were up to me, I'd leave the function signature alone, and just add
> (void) miinfo; /* unused parameter */
> to the function code. It seems perfectly reasonable to have that
> function argument, and a little weird not to have it.
>
>
I think, we should only do that if at all there is any circumstances under
which 'miinfo' might be used otherwise it would good to remove it unless
there is some specific reason for having it. As an example, please refer to
the following code in printTableAddHeader() or printTableAddCell().

#ifndef ENABLE_NLS
(void) translate; /* unused parameter */
#endif

The function argument *translate* has been marked as unsed but only for
non-nls build which means it will be used if it is nls enabled build. But,
I do not see any such requirement in our case. Please let me know if I
missing anything here.

Thanks,

--
With Regards,
Ashutosh Sharma
EnterpriseDB:*http://www.enterprisedb.com <http://www.enterprisedb.com/>*