On Tue, Nov 17, 2020 at 2:38 AM Heikki Linnakangas <hlinn...@iki.fi> wrote:
>
> Thanks for feedback, attached is a new patch version.
>
> On 11/11/2020 21:49, Soumyadeep Chakraborty wrote:
> > On Tue, Nov 3, 2020 at 1:30 AM Heikki Linnakangas <hlinn...@iki.fi> wrote:
> >> I also split/duplicated the CopyStateData struct into CopyFromStateData
> >> and CopyToStateData. Many of the fields were common, but many were not,
> >> and I think some duplication is nicer than a struct where you use some
> >> fields and others are unused. I put the common formatting options into a
> >> new CopyFormatOptions struct.
> >
> > Would we be better off if we sub-struct CopyState <- Copy{From,To}State?
> > Like this:
> > typedef struct Copy{From|To}StateData
> > {
> > CopyState cs;
> > // Fields specific to COPY FROM/TO follow..
> > }
>
> Hmm. I don't think that would be better. There isn't actually that much
> in common between CopyFromStateData and CopyToStateData, and a little
> bit of duplication seems better.
>Fair. > > 6. > > > >> /* create workspace for CopyReadAttributes results */ > >> if (!cstate->opts.binary) > > > > Can we replace this if with an else? > > Seems better as it is IMO, the if- and else-branches are not really > related to each other, even though they both happen to be conditioned on > cstate->opts.binary. Fair. > Fixed all the other things you listed, fixed a bug in setting > 'file_encoding', and trimmed down the #includes. > Thanks! LGTM! Marking as Ready for Committer. Regards, Soumyadeep (VMware)
