Pavel,
* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> Now I fixed these issues and I hope so it will work on all platforms
As mentioned on the commitfest application, this needs documentation.
That is not the responsibility of the committer; if you need help, then
please ask for it.
I've also done a quick review of it.
The massive if() block being added to execQual.c:ExecMakeFunctionResult
really looks like it might be better pulled out into a function of its
own.
What does "use element_type depends for dimensions" mean and why is it
a ToDo? When will it be done?
Overall, the comments really need to be better. Things like this:
+ /* create short lived copies of fmgr data */
+ fmgr_info_copy(&sfinfo, fcinfo->flinfo, fcinfo->flinfo->fn_mcxt);
+ memcpy(scinfo, fcinfo, sizeof(FunctionCallInfoData));
+ scinfo->flinfo = &sfinfo;
Really don't cut it, imv. *Why* are we creating a copy of the fmgr
data? Why does it need to be short lived? And what is going to happen
later when you do this?:
fcinfo = scinfo;
MemoryContextSwitchTo(oldContext);
Is there any reason to worry about the fact that fcache->fcinfo_data no
longer matches the fcinfo that we're working on? What if there are
other references made to it in the future? These memcpy's and pointer
foolishness really don't strike me as being a well thought out approach.
There are other similar comments throughout which really need to be
rewritten to address why we're doing something, not what is being done-
you can read the code for that.
Marking this Waiting on Author.
Thanks,
Stephen
