Hi,
On 2019-09-30 09:14:45 -0700, Soumyadeep Chakraborty wrote:
> I don't feel very strongly about the changes I proposed.
>
> > > I completely agree, that was an important consideration.
> > >
> > > I had some purely cosmetic suggestions:
> > > 1. Rename ExecComputeSlotInfo to eliminate the need for the asserts.
> >
> > How does renaming it do so? I feel like the asserts are a good idea
> > independent of anything else?
>
> I felt that encoding the restriction that the function is meant to be called
> only in the context of fetch operations in the function name itself
> ensured that we don't call it from a non-fetch operation - something the
> asserts within ExecComputeSlotInfo() are guarding against.
>
> >
> > > 2. Extract return value to a bool variable for slightly better
> > > readability.
> >
> > To me that seems clearly worse. The variable doesn't add anything, but
> > needing to track more state.>
>
> Agreed.
I pushed this to master. Thanks for your contribution!
Regards,
Andres