Re: warn_unused_results
On 2020-11-10 04:34, Michael Paquier wrote: I am not sure about the addition of repalloc(), as it is quite obvious that one has to use its result. Lists are fine, these are proper to PG internals and beginners tend to be easily confused in the way to use them. realloc() is listed in the GCC documentation as the reason this option exists, and glibc tags its realloc() with this attribute, so doing the same for repalloc() seems sensible. Good point. committed -- Peter Eisentraut 2ndQuadrant, an EDB company https://www.2ndquadrant.com/
Re: warn_unused_results
On Mon, Nov 09, 2020 at 08:23:31AM +0100, Peter Eisentraut wrote: > On 2020-11-09 07:56, Michael Paquier wrote: >> This is accepted by clang, and MSVC has visibly an equivalent for >> that, as of VS 2012: >> #elif defined(_MSC_VER) && (_MSC_VER >= 1700) >> #define pg_nodiscard _Check_return_ >> We don't care about the 1700 condition as we support only >= 1800 on >> HEAD, and in this case the addition of pg_nodiscard would be required >> on the definition and the declaration. Should it be added? It is >> much more invasive than the gcc/clang equivalent though.. > > AFAICT from the documentation, this only applies for special "analyze" runs, > not as a normal compiler warning. Do we have any support for analyze runs > with MSVC? You can run them by passing down /p:RunCodeAnalysis=true to MSBFLAGS when calling the build script. There are more options like /p:CodeAnalysisRuleSet to define a set of rules. By default the output is rather noisy though now that I look at it. And having to add the flag to the definition and the declaration is annoying, so what you are doing would be enough without MSVC. >> I am not sure >> about the addition of repalloc(), as it is quite obvious that one has >> to use its result. Lists are fine, these are proper to PG internals >> and beginners tend to be easily confused in the way to use them. > > realloc() is listed in the GCC documentation as the reason this option > exists, and glibc tags its realloc() with this attribute, so doing the same > for repalloc() seems sensible. Good point. -- Michael signature.asc Description: PGP signature
Re: warn_unused_results
On 2020-11-09 07:56, Michael Paquier wrote: This is accepted by clang, and MSVC has visibly an equivalent for that, as of VS 2012: #elif defined(_MSC_VER) && (_MSC_VER >= 1700) #define pg_nodiscard _Check_return_ We don't care about the 1700 condition as we support only >= 1800 on HEAD, and in this case the addition of pg_nodiscard would be required on the definition and the declaration. Should it be added? It is much more invasive than the gcc/clang equivalent though.. AFAICT from the documentation, this only applies for special "analyze" runs, not as a normal compiler warning. Do we have any support for analyze runs with MSVC? FWIW, I saw an extra case with parsePGArray() today. AFAICT, that's more in the category of checking for error returns, which is similar to the "fortify" options that some environments have for checking the return of fwrite() etc. I am not sure about the addition of repalloc(), as it is quite obvious that one has to use its result. Lists are fine, these are proper to PG internals and beginners tend to be easily confused in the way to use them. realloc() is listed in the GCC documentation as the reason this option exists, and glibc tags its realloc() with this attribute, so doing the same for repalloc() seems sensible. -- Peter Eisentraut 2ndQuadrant, an EDB company https://www.2ndquadrant.com/
Re: warn_unused_results
On Sat, Oct 17, 2020 at 08:57:51AM +0200, Peter Eisentraut wrote: > 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 seen as well this stuff being a source of confusion in the past. > 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. +#ifdef __GNUC__ +#define pg_nodiscard __attribute__((warn_unused_result)) +#else +#define pg_nodiscard +#endif This is accepted by clang, and MSVC has visibly an equivalent for that, as of VS 2012: #elif defined(_MSC_VER) && (_MSC_VER >= 1700) #define pg_nodiscard _Check_return_ We don't care about the 1700 condition as we support only >= 1800 on HEAD, and in this case the addition of pg_nodiscard would be required on the definition and the declaration. Should it be added? It is much more invasive than the gcc/clang equivalent though.. > 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. FWIW, I saw an extra case with parsePGArray() today. I am not sure about the addition of repalloc(), as it is quite obvious that one has to use its result. Lists are fine, these are proper to PG internals and beginners tend to be easily confused in the way to use them. -- Michael signature.asc Description: PGP signature
Re: warn_unused_results
On 2020-10-17 17:58, Tom Lane wrote: Peter Eisentraut writes: 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. +1 in principle (I've not read the patch in detail); but I wonder what pgindent does with these added keywords. pgindent doesn't seem to want to change anything about the patched files as I had sent them. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: warn_unused_results
Peter Eisentraut writes: > 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. +1 in principle (I've not read the patch in detail); but I wonder what pgindent does with these added keywords. regards, tom lane
warn_unused_results
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 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 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);
