warn_unused_results

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

On 2020-10-17 17:58, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

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

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/

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

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/