Support isEmptyStringInfo
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 t45688_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 t45688_1 && git checkout t45688_1Patchset v1 (message #1) is on t45688_1
Hi All,
We have (StringInfo::len == 0) checks at many places. I thought it
would be better to wrap that into a function isEmptyStringInfo() to
make those checks more readable and also abstract the logic to check
emptiness of a StringInfo. I think this will be useful to extensions
outside core which also have these checks. They won't need to worry
about that logic/code being changed in future; rare but not impossible
case.
Probably we should have similar support for PQExpBuffer as well, which
will be more useful to hide the internals of PQExpBuffer from client
code. But I haven't included those changes in this patch. I can do
that if hackers like the idea.
--
Best Wishes,
Ashutosh Bapat
On Wed, Mar 23, 2022 at 8:33 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
We have (StringInfo::len == 0) checks at many places. I thought it
would be better to wrap that into a function isEmptyStringInfo() to
make those checks more readable and also abstract the logic to check
emptiness of a StringInfo. I think this will be useful to extensions
outside core which also have these checks. They won't need to worry
about that logic/code being changed in future; rare but not impossible
case.
I think that the code is perfectly readable as it is and that this
change makes it less so.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
I think that the code is perfectly readable as it is and that this
change makes it less so.
Yeah, after a quick look through this patch I'm unimpressed too.
The new code is strictly longer, and it requires the introduction
of distracting "!" and "&" operators in many places.
regards, tom lane
At Wed, 23 Mar 2022 10:13:43 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
Robert Haas <robertmhaas@gmail.com> writes:
I think that the code is perfectly readable as it is and that this
change makes it less so.Yeah, after a quick look through this patch I'm unimpressed too.
The new code is strictly longer, and it requires the introduction
of distracting "!" and "&" operators in many places.
The struct members are not private at all. In that sense StringInfo
is not a kind of class of C/Java but like a struct of C/C++ at least
to me. I think encapsulating only ".len == 0" doesn't help. Already
in many places we pull out buf.data to use it separately from buf, we
have a dozen of instances of "buf.len (<|>|<=|>=) <some length>" and
even "buf.data[buf.len - 1] == '\n'"
About read-easiness, isEmptyStringInfo(str) slightly spins my eyes
than str->len == 0.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center