On 19.03.24 17:13, Peter Eisentraut wrote:
> On 11.03.24 21:52, Matthias van de Meent wrote:
>>> * v7-0003-gen_node_support.pl-Mark-location-fields-as-type-.patch
>>>
>>> This looks sensible, but maybe making Location a global type is a bit
>>> much? Maybe something more specific like ParseLocation, or ParseLoc, to
>>> keep it under 12 characters.
>> I've gone with ParseLoc in the attached v8 patchset.
>
> I have committed this one.
Next, I was looking at
v8-0003-pg_node_tree-Don-t-store-query-text-locations-in-.patch. After
applying that, I was looking how many uses of nodeToString() (with
locations) were left. I think your patch forgot to convert a number of
them, and there also might have been a few new ones that came in with
other recent patches. Might be hard to make sure all new developments
do this right. Plus, there are various mentions in the documentation
that should be updated. After considering all that, there weren't
really many callers of nodeToString() left. It's really only debugging
support in postgres.c and print.c, and a few places were it doesn't
matter, like the few places where it initializes "cooked expressions",
which were in turn already stripped of location fields at some earlier time.
So anyway, my idea was that we should turn this around and make
nodeToString() always drop location information, and instead add
nodeToStringWithLocations() for the few debugging uses. And this would
also be nice because then it matches exactly with the existing
stringToNodeWithLocations().
Attached patch shows this.