Thread: use-regular-expressions-to-simplify-less_greater-and-not_equals.patch
we can use regular expressions (<>|!=) to cover "<>" and "!=". There is no
need to have two definitions less_greater and not_equals, because it will confuse developer.
So, we can use only not_equals to cover this operator set.
need to have two definitions less_greater and not_equals, because it will confuse developer.
So, we can use only not_equals to cover this operator set.
Please review my patch.
Thanks.
Attachment
"=?UTF-8?B?5a2Z6K+X5rWpKOaAneaJjSk=?=" <sunshihao.ssh@alibaba-inc.com> writes:
> we can use regular expressions (<>|!=) to cover "<>" and "!=". There is no
> need to have two definitions less_greater and not_equals, because it will confuse developer.
> So, we can use only not_equals to cover this operator set.
I do not find this an improvement. Yeah, it's a bit shorter, but it's
less clear; not least because the comment explaining the <>-means-!=
behavior is no longer anywhere near the code that implements that
behavior. It would also get in the way if we ever had reason to treat <>
and != as something other than exact equivalents.
regards, tom lane
Re: use-regular-expressions-to-simplify-less_greater-and-not_equals.patch
From
"David G. Johnston"
Date:
On Tue, Aug 10, 2021 at 8:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"孙诗浩(思才)" <sunshihao.ssh@alibaba-inc.com> writes:
> we can use regular expressions (<>|!=) to cover "<>" and "!=". There is no
> need to have two definitions less_greater and not_equals, because it will confuse developer.
> So, we can use only not_equals to cover this operator set.
I do not find this an improvement.
Agreed, though mostly on a separation of responsibilities principle. Labelling the distinctive tokens and then assigning them meaning are two different things.
Yeah, it's a bit shorter, but it's
less clear;
not least because the comment explaining the <>-means-!=
behavior is no longer anywhere near the code that implements that
behavior.
The patch should just remove the comment for not_equals as well: if both tokens are given the same name they would have to be treated identically. The fact they have the same name is clearer in that the equivalency knowledge is immediate and impossible to miss (if one doesn't go and check how "less_greater" and "not_equal" are interpreted.)
It would also get in the way if we ever had reason to treat <>
and != as something other than exact equivalents.
This is unconvincing both on the odds of being able to treat them differently and the fact that the degree of effort to overcome this obstacle seems minimal - about the same amount as reverting this patch, not accounting for the coding of the new behavior.
In short, for me, is a principled separation of concerns worth the slight loss is clarity? I'll stick to my status quo choice, but not strongly. Some more context as to exactly what kind of confusion this setup is causing could help tip the scale.
David J.
On 8/10/21 11:27 PM, 孙诗浩(思才) wrote: > we can use regular expressions (<>|!=) to cover "<>" and "!=". > There is no > need to have two definitions less_greater and not_equals, because it > will confuse developer. > So, we can use only not_equals to cover this operator set. > > I don't understand the problem being solved here. This looks like a change with no discernable benefit. I don't agree that the current code is substantially more likely to confuse developers. And the lexer is not something that gets a lot of change. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> On 8/10/21 11:27 PM, 孙诗浩(思才) wrote:
>> we can use regular expressions (<>|!=) to cover "<>" and "!=".
> I don't understand the problem being solved here. This looks like a
> change with no discernable benefit. I don't agree that the current code
> is substantially more likely to confuse developers. And the lexer is not
> something that gets a lot of change.
By my count, the response to this has been two definite "no"s, one
leaning-to-no, and nobody speaking in favor. Nor has the submitter
provided a stronger argument for it. I'm going to mark this
rejected in the CF app.
regards, tom lane