Re: Residual cpluspluscheck issues - Mailing list pgsql-hackers

Hi Tom,

So it's been 17 months since you sent this email, so I'm not sure that
nothing has happened (off list or in the code base), but...

On Sun, Jun 2, 2019 at 9:53 AM Tom Lane wrote:
> * On FreeBSD 12, I get
>
> /home/tgl/pgsql/src/include/utils/hashutils.h:23:23: warning: 'register' storage
>       class specifier is deprecated and incompatible with C++17
>       [-Wdeprecated-register]
> extern Datum hash_any(register const unsigned char *k, register int keylen);
>                       ^~~~~~~~~
> /home/tgl/pgsql/src/include/utils/hashutils.h:23:56: warning: 'register' storage
>       class specifier is deprecated and incompatible with C++17
>       [-Wdeprecated-register]
> extern Datum hash_any(register const unsigned char *k, register int keylen);
>                                                        ^~~~~~~~~
> /home/tgl/pgsql/src/include/utils/hashutils.h:24:32: warning: 'register' storage
>       class specifier is deprecated and incompatible with C++17
>       [-Wdeprecated-register]
> extern Datum hash_any_extended(register const unsigned char *k,
>                                ^~~~~~~~~
> /home/tgl/pgsql/src/include/utils/hashutils.h:25:11: warning: 'register' storage
>       class specifier is deprecated and incompatible with C++17
>       [-Wdeprecated-register]
>                                                            register int ...
>                                                            ^~~~~~~~~
>
> which I'm inclined to think means we should drop those register keywords.

I think this is a warning from Clang, right? You can get the same
warning on macOS if you use the upstream Clang where the default value
of -std for clang++ has been gnu++14 since LLVM 6.0 (not AppleClang,
which carries a proprietary patch that simply reverts the bump, but they
didn't even bother to patch the manpage).

I'm running into the same (well similar) warnings when running
cpluspluscheck with GCC 11. Upon closer inspection, this is because In
GCC 11, the default value of -std has been bumped to gnu++17. IOW, I
would've gotten the same warning had I just configured with CXX="g++-10
-std=gnu++17". The g++ warnings look like the following:

gcc> In file included from ./src/include/port/atomics.h:70,
gcc>                  from ./src/include/storage/lwlock.h:21,
gcc>                  from ./src/include/storage/lock.h:23,
gcc>                  from ./src/include/storage/proc.h:21,
gcc>                  from ./src/include/storage/shm_mq.h:18,
gcc>                  from ./src/test/modules/test_shm_mq/test_shm_mq.h:18,
gcc>                  from /tmp/cpluspluscheck.AxICnl/test.cpp:3:
gcc> ./src/include/port/atomics/arch-x86.h: In function 'bool
pg_atomic_test_set_flag_impl(volatile pg_atomic_flag*)':
gcc> ./src/include/port/atomics/arch-x86.h:143:16: warning: ISO C++17
does not allow 'register' storage class specifier [-Wregister]
gcc>   143 |  register char _res = 1;
gcc>       |                ^~~~
gcc> In file included from ./src/include/storage/spin.h:54,
gcc>                  from ./src/test/modules/test_shm_mq/test_shm_mq.h:19,
gcc>                  from /tmp/cpluspluscheck.AxICnl/test.cpp:3:
gcc> ./src/include/storage/s_lock.h: In function 'int tas(volatile slock_t*)':
gcc> ./src/include/storage/s_lock.h:226:19: warning: ISO C++17 does
not allow 'register' storage class specifier [-Wregister]
gcc>   226 |  register slock_t _res = 1;
gcc>       |                   ^~~~

I think this is a problem worth solving: C++ 17 is removing the register
keyword, and C++ code that includes our headers have the difficult
choices of:

1) With a pre-C++ 17 compiler that forewarns the deprecation, find a
compiler-specific switch to turn off the warning

2) With a compiler that defaults to C++ 17 or later (major compiler
vendors are upgrading the default to C++17):

   2a) find a switch to explicitly downgrade to C++ 14 or below (and
   then possibly jump back to solving problem number 1).

   2b) find a compiler-specific switch to stay in the post- C++ 17 mode,
   but somehow "turn off" the removal of register keyword. This is
   particularly cringe-y because the C++ programs themselves have to be
   non-formant through a header we supply.

We can either drop the register keywords here (I wonder what the impact
on code generation would be, but it'll be hard to examine, given we'll
need to examine _every_ instance of generated code for an inline
function), or maybe consider hiding those sections with "#ifndef
__cplusplus" (provided that we believe there's not much of a reason for
the including code to call these functions, just throwing out uneducated
guesses here).

Do we have a clear decision of what we want to do here? How can I
contribute?

Cheers,
Jesse