On 2019-05-17 11:09:41 +0530, Ashutosh Sharma wrote: > On Fri, May 17, 2019 at 3:10 AM Alvaro Herrera <alvhe...@2ndquadrant.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'd do that, or simply nothing. We've plenty of unused args, so I don't see much point in these kind of "unused parameter" warning suppressions in isolated places. > 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. It seems like it could entirely be reasonable to e.g. reuse slots across partitions, which'd require CopyMultiInsertInfo to be around. Also, from a consistency point, it seems the caller doesn't need to know whether all the necessary information is in the ResultRelInfo and not in CopyMultiInsertInfo for *some* of the CopyMultiInsertInfo functions. Greetings, Andres Freund
