use-regular-expressions-to-simplify-less_greater-and-not_equals.patch
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t44666_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t44666_1 && git checkout t44666_1Patchset v1 (message #1) is on t44666_1
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.
Please review my patch.
Thanks.
"=?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
On Tue, Aug 10, 2021 at 8:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"=?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.
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