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
