Support isEmptyStringInfo
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
Attachments:
0001-isEmptyStringInfo-to-check-whether-StringInfo-has-no.patchtext/x-patch; charset=US-ASCII; name=0001-isEmptyStringInfo-to-check-whether-StringInfo-has-no.patchDownload
From 0ff57f4b8fffff0094d581806f5553397c286736 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Date: Wed, 23 Mar 2022 17:40:44 +0530
Subject: [PATCH] isEmptyStringInfo() to check whether StringInfo has no data
Add a function isEmptyStringInfo() which returns true if there is no
data in the given StringInfo buffer. False otherwise. This hides the
logic to check emptiness in stringinfo module.
Ashutosh Bapat
---
src/backend/catalog/objectaddress.c | 4 ++--
src/backend/catalog/pg_shdepend.c | 2 +-
src/backend/commands/copyfromparse.c | 2 +-
src/backend/commands/copyto.c | 2 +-
src/backend/commands/explain.c | 4 ++--
src/backend/utils/adt/ruleutils.c | 4 ++--
src/backend/utils/fmgr/dfmgr.c | 4 ++--
src/include/lib/stringinfo.h | 10 ++++++++++
8 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index f30c742d48..5aa97bb485 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -4066,7 +4066,7 @@ getObjectDescription(const ObjectAddress *object, bool missing_ok)
}
/* an empty buffer is equivalent to no object found */
- if (buffer.len == 0)
+ if (isEmptyStringInfo(&buffer))
return NULL;
return buffer.data;
@@ -5938,7 +5938,7 @@ getObjectIdentityParts(const ObjectAddress *object,
else
{
/* an empty buffer is equivalent to no object found */
- if (buffer.len == 0)
+ if (isEmptyStringInfo(&buffer))
{
Assert((objname == NULL || *objname == NIL) &&
(objargs == NULL || *objargs == NIL));
diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index 3e8fa008b9..597927011c 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -810,7 +810,7 @@ checkSharedDependencies(Oid classId, Oid objectId,
pfree(objects);
list_free_deep(remDeps);
- if (descs.len == 0)
+ if (isEmptyStringInfo(&descs))
{
pfree(descs.data);
pfree(alldescs.data);
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index baf328b620..ec4eac0ea6 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -776,7 +776,7 @@ NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
* characters, we act as though it was newline followed by EOF, ie,
* process the line and then exit loop on next iteration.
*/
- if (done && cstate->line_buf.len == 0)
+ if (done && isEmptyStringInfo(&(cstate->line_buf))
return false;
/* Parse the line into de-escaped field values */
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 55c38b04c4..9e6d094764 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -156,7 +156,7 @@ static void
SendCopyEnd(CopyToState cstate)
{
/* Shouldn't have any unsent data */
- Assert(cstate->fe_msgbuf->len == 0);
+ Assert(isEmptyStringInfo(&(cstate->fe_msgbuf));
/* Send Copy Done message */
pq_putemptymessage('c');
}
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 9f632285b6..4b9c681c4b 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -4174,7 +4174,7 @@ ExplainOpenWorker(int n, ExplainState *es)
*/
if (es->format == EXPLAIN_FORMAT_TEXT)
{
- if (es->str->len == 0)
+ if (isEmptyStringInfo(es->str))
{
ExplainIndentText(es);
appendStringInfo(es->str, "Worker %d: ", n);
@@ -4870,7 +4870,7 @@ static void
ExplainIndentText(ExplainState *es)
{
Assert(es->format == EXPLAIN_FORMAT_TEXT);
- if (es->str->len == 0 || es->str->data[es->str->len - 1] == '\n')
+ if (isEmptyStringInfo(es->str) || es->str->data[es->str->len - 1] == '\n')
appendStringInfoSpaces(es->str, es->indent * 2);
}
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b16526e65e..43028dbda8 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -607,7 +607,7 @@ pg_get_ruledef_worker(Oid ruleoid, int prettyFlags)
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
- if (buf.len == 0)
+ if (isEmptyStringInfo(&buf))
return NULL;
return buf.data;
@@ -803,7 +803,7 @@ pg_get_viewdef_worker(Oid viewoid, int prettyFlags, int wrapColumn)
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, "SPI_finish failed");
- if (buf.len == 0)
+ if (isEmptyStringInfo(&buf))
return NULL;
return buf.data;
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 050da78080..ef4e3420b0 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -382,7 +382,7 @@ incompatible_module_error(const char *libname,
}
if (module_magic_data->float8byval != magic_data.float8byval)
{
- if (details.len)
+ if (!isEmptyStringInfo(&details))
appendStringInfoChar(&details, '\n');
appendStringInfo(&details,
_("Server has FLOAT8PASSBYVAL = %s, library has %s."),
@@ -390,7 +390,7 @@ incompatible_module_error(const char *libname,
module_magic_data->float8byval ? "true" : "false");
}
- if (details.len == 0)
+ if (isEmptyStringInfo(&details))
appendStringInfoString(&details,
_("Magic block has unexpected length or padding difference."));
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 9b755c4883..0e659b976a 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -158,4 +158,14 @@ extern void appendBinaryStringInfoNT(StringInfo str,
*/
extern void enlargeStringInfo(StringInfo str, int needed);
+/*------------------------
+ * isEmptyStringInfo
+ * Returns true if the given StringInfo does not have any data in it. False
+ * otherwise.
+ */
+static inline isEmptyStringInfo(StringInfo str)
+{
+ return str->len == 0;
+}
+
#endif /* STRINGINFO_H */
--
2.25.1
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