On 05.01.26 17:18, Bertrand Drouvot wrote:
>> For patch 0001, this seems good, but I wonder why your patch catches some
>> cases and not some other similar ones. For example, in
>> src/backend/access/brin/brin_minmax_multi.c, you change compare_distances(),
>> but not the very similar compare_expanded_ranges() and compare_values()
>> nearby.
> The initial patch was filtering out more complex functions that would need more
> study. The idea was to look at those later on.
>
> Now, about compare_expanded_ranges() and compare_values(), that's right that
> those functions have similar patterns and could be included and their "extra"
> study is simple as realizing that minval and maxval are Datum (so uint64_t),
> are pass by values to FunctionCall2Coll() so that it can not modify them.
>
> So, better to be consistent within the same file, those 2 functions have been
> added in the attached.
>
> Also I've added the changes for sort_item_compare() even this is a thin wrapper
> so that the changes are consistent accross the mcv.c file too.
>
> Now all the remaining ones reported by [1] are in files not touched by the attached,
> making it consistent on a per file basis.
>
> Note that it does not take care at all of "nearby" places where we could remove
> explicit casts when assigning from void pointers (for example the arg parameter
> in compare_expanded_ranges() and compare_values()) as I think that could be
> worth a dedicated project as stated in [2].
I have committed the remaining two patches.
I had in my own work parked a few changes that were similar to the ones
in your patch 0001, so I threw them in there as well.
