Forgetting to assign the return value of list APIs such as lappend() is
a perennial favorite. The compiler can help point out such mistakes.
GCC has an attribute warn_unused_results. Also C++ has standardized
this under the name "nodiscard", and C has a proposal to do the same
[0]. In my patch I call the symbol pg_nodiscard, so that perhaps in a
distant future one only has to do s/pg_nodiscard/nodiscard/ or something
similar. Also, the name is short enough that it doesn't mess up the
formatting of function declarations too much.
I have added pg_nodiscard decorations to all the list functions where I
found it sensible, as well as repalloc() for good measure, since
realloc() is usually mentioned as an example where this function
attribute is useful.
I have found two places in the existing code where this creates
warnings. Both places are correct as is, but make assumptions about the
internals of the list APIs and it seemed better just to fix the warning
than to write a treatise about why it's correct as is.
[0]: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2051.pdf
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 32a9bc5cd2c63500670b663964e878e4474ce257 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Sat, 17 Oct 2020 08:38:39 +0200
Subject: [PATCH 1/3] Add pg_nodiscard function declaration specifier
pg_nodiscard means the compiler should warn if the result of a
function call is ignored. The name "nodiscard" is chosen in alignment
with (possibly future) C and C++ standards. It maps to the GCC
attribute warn_unused_result.
---
src/include/c.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/include/c.h b/src/include/c.h
index 9cd67f8f76..d5dc3632f7 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -111,6 +111,18 @@
#define pg_attribute_unused()
#endif
+/*
+ * pg_nodiscard means the compiler should warn if the result of a function
+ * call is ignored. The name "nodiscard" is chosen in alignment with
+ * (possibly future) C and C++ standards. For maximum compatibility, use it
+ * as a function declaration specifier, so it goes before the return type.
+ */
+#ifdef __GNUC__
+#define pg_nodiscard __attribute__((warn_unused_result))
+#else
+#define pg_nodiscard
+#endif
+
/*
* Append PG_USED_FOR_ASSERTS_ONLY to definitions of variables that are only
* used in assert-enabled builds, to avoid compiler warnings about unused
--
2.28.0
From ff90b03e24011f59c2a0fa7c3569c64e1189a028 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Sat, 17 Oct 2020 08:38:39 +0200
Subject: [PATCH 2/3] Add pg_nodiscard decorations to some functions
Especially for the list API such as lappend() forgetting to assign the
return value is a common problem.
---
src/include/nodes/pg_list.h | 62 ++++++++++++++++++-------------------
src/include/utils/palloc.h | 4 +--
2 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h
index ec231010ce..cda77a841e 100644
--- a/src/include/nodes/pg_list.h
+++ b/src/include/nodes/pg_list.h
@@ -521,36 +521,36 @@ extern List *list_make3_impl(NodeTag t, ListCell datum1,
ListCell datum2,
extern List *list_make4_impl(NodeTag t, ListCell datum1, ListCell datum2,
ListCell datum3,
ListCell datum4);
-extern List *lappend(List *list, void *datum);
-extern List *lappend_int(List *list, int datum);
-extern List *lappend_oid(List *list, Oid datum);
+extern pg_nodiscard List *lappend(List *list, void *datum);
+extern pg_nodiscard List *lappend_int(List *list, int datum);
+extern pg_nodiscard List *lappend_oid(List *list, Oid datum);
-extern List *list_insert_nth(List *list, int pos, void *datum);
-extern List *list_insert_nth_int(List *list, int pos, int datum);
-extern List *list_insert_nth_oid(List *list, int pos, Oid datum);
+extern pg_nodiscard List *list_insert_nth(List *list, int pos, void *datum);
+extern pg_nodiscard List *list_insert_nth_int(List *list, int pos, int datum);
+extern pg_nodiscard List *list_insert_nth_oid(List *list, int pos, Oid datum);
-extern List *lcons(void *datum, List *list);
-extern List *lcons_int(int datum, List *list);
-extern List *lcons_oid(Oid datum, List *list);
+extern pg_nodiscard List *lcons(void *datum, List *list);
+extern pg_nodiscard List *lcons_int(int datum, List *list);
+extern pg_nodiscard List *lcons_oid(Oid datum, List *list);
-extern List *list_concat(List *list1, const List *list2);
-extern List *list_concat_copy(const List *list1, const List *list2);
+extern pg_nodiscard List *list_concat(List *list1, const List *list2);
+extern pg_nodiscard List *list_concat_copy(const List *list1, const List
*list2);
-extern List *list_truncate(List *list, int new_size);
+extern pg_nodiscard List *list_truncate(List *list, int new_size);
extern bool list_member(const List *list, const void *datum);
extern bool list_member_ptr(const List *list, const void *datum);
extern bool list_member_int(const List *list, int datum);
extern bool list_member_oid(const List *list, Oid datum);
-extern List *list_delete(List *list, void *datum);
-extern List *list_delete_ptr(List *list, void *datum);
-extern List *list_delete_int(List *list, int datum);
-extern List *list_delete_oid(List *list, Oid datum);
-extern List *list_delete_first(List *list);
-extern List *list_delete_last(List *list);
-extern List *list_delete_nth_cell(List *list, int n);
-extern List *list_delete_cell(List *list, ListCell *cell);
+extern pg_nodiscard List *list_delete(List *list, void *datum);
+extern pg_nodiscard List *list_delete_ptr(List *list, void *datum);
+extern pg_nodiscard List *list_delete_int(List *list, int datum);
+extern pg_nodiscard List *list_delete_oid(List *list, Oid datum);
+extern pg_nodiscard List *list_delete_first(List *list);
+extern pg_nodiscard List *list_delete_last(List *list);
+extern pg_nodiscard List *list_delete_nth_cell(List *list, int n);
+extern pg_nodiscard List *list_delete_cell(List *list, ListCell *cell);
extern List *list_union(const List *list1, const List *list2);
extern List *list_union_ptr(const List *list1, const List *list2);
@@ -567,24 +567,24 @@ extern List *list_difference_ptr(const List *list1, const
List *list2);
extern List *list_difference_int(const List *list1, const List *list2);
extern List *list_difference_oid(const List *list1, const List *list2);
-extern List *list_append_unique(List *list, void *datum);
-extern List *list_append_unique_ptr(List *list, void *datum);
-extern List *list_append_unique_int(List *list, int datum);
-extern List *list_append_unique_oid(List *list, Oid datum);
+extern pg_nodiscard List *list_append_unique(List *list, void *datum);
+extern pg_nodiscard List *list_append_unique_ptr(List *list, void *datum);
+extern pg_nodiscard List *list_append_unique_int(List *list, int datum);
+extern pg_nodiscard List *list_append_unique_oid(List *list, Oid datum);
-extern List *list_concat_unique(List *list1, const List *list2);
-extern List *list_concat_unique_ptr(List *list1, const List *list2);
-extern List *list_concat_unique_int(List *list1, const List *list2);
-extern List *list_concat_unique_oid(List *list1, const List *list2);
+extern pg_nodiscard List *list_concat_unique(List *list1, const List *list2);
+extern pg_nodiscard List *list_concat_unique_ptr(List *list1, const List
*list2);
+extern pg_nodiscard List *list_concat_unique_int(List *list1, const List
*list2);
+extern pg_nodiscard List *list_concat_unique_oid(List *list1, const List
*list2);
extern void list_deduplicate_oid(List *list);
extern void list_free(List *list);
extern void list_free_deep(List *list);
-extern List *list_copy(const List *list);
-extern List *list_copy_tail(const List *list, int nskip);
-extern List *list_copy_deep(const List *oldlist);
+extern pg_nodiscard List *list_copy(const List *list);
+extern pg_nodiscard List *list_copy_tail(const List *list, int nskip);
+extern pg_nodiscard List *list_copy_deep(const List *oldlist);
typedef int (*list_sort_comparator) (const ListCell *a, const ListCell *b);
extern void list_sort(List *list, list_sort_comparator cmp);
diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h
index cc356a6372..c801e12478 100644
--- a/src/include/utils/palloc.h
+++ b/src/include/utils/palloc.h
@@ -77,7 +77,7 @@ extern void *MemoryContextAllocExtended(MemoryContext context,
extern void *palloc(Size size);
extern void *palloc0(Size size);
extern void *palloc_extended(Size size, int flags);
-extern void *repalloc(void *pointer, Size size);
+extern pg_nodiscard void *repalloc(void *pointer, Size size);
extern void pfree(void *pointer);
/*
@@ -95,7 +95,7 @@ extern void pfree(void *pointer);
/* Higher-limit allocators. */
extern void *MemoryContextAllocHuge(MemoryContext context, Size size);
-extern void *repalloc_huge(void *pointer, Size size);
+extern pg_nodiscard void *repalloc_huge(void *pointer, Size size);
/*
* Although this header file is nominally backend-only, certain frontend
--
2.28.0
From 34b96535c268fac21034d2076c54f10da7e9a3b3 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Sat, 17 Oct 2020 08:38:39 +0200
Subject: [PATCH 3/3] Silence some pg_nodiscard warnings
Two cases violated pg_nodiscard declarations on the list API. While
the code was technically correct, it relied on internal knowledge of
the list implementation, and the code wasn't really gaining anything
that way, so just fix it by assigning the return value properly.
---
src/backend/commands/lockcmds.c | 2 +-
src/backend/parser/analyze.c | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index d8cafc42bb..08c4de1d81 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -263,7 +263,7 @@ LockViewRecurse(Oid reloid, LOCKMODE lockmode, bool nowait,
List *ancestor_views
LockViewRecurse_walker((Node *) viewquery, &context);
- (void) list_delete_last(context.ancestor_views);
+ context.ancestor_views = list_delete_last(context.ancestor_views);
table_close(view, NoLock);
}
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index c159fb2957..4aab2eddcb 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1486,8 +1486,7 @@ transformValuesClause(ParseState *pstate, SelectStmt
*stmt)
Node *col = (Node *) lfirst(lc);
List *sublist = lfirst(lc2);
- /* sublist pointer in exprsLists won't need adjustment
*/
- (void) lappend(sublist, col);
+ sublist = lappend(sublist, col);
}
list_free(colexprs[i]);
}
--
2.28.0
