Horton

Thread

Commits

  1. Avoid conflicts with library versions of inet_net_ntop() and friends.

  2. Fix incidental warnings from cpluspluscheck.

  1. Residual cpluspluscheck issues

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-02T16:53:14Z

    cpluspluscheck's expanded coverage is now passing cleanly for me on
the macOS laptop I was testing it with at PGCon.  But on returning
home, I find there's still some issues on some other boxes:

* On Linux (at least Fedora and RHEL), I get variants of this:

/usr/include/arpa/inet.h:84: error: declaration of 'char* inet_net_ntop(int, const void*, int, char*, size_t) throw ()' throws different exceptions
/home/postgres/pgsql/src/include/port.h:506: error: from previous declaration 'char* inet_net_ntop(int, const void*, int, char*, size_t)'

That's because /usr/include/arpa/inet.h declares it as

extern char *inet_net_ntop (int __af, const void *__cp, int __bits,
                            char *__buf, size_t __len) __THROW;

and of course when a C++ compiler reads that, __THROW will expand as
something nonempty.

One possible fix for that is to teach configure to test whether
arpa/inet.h provides a declaration, and not compile our own declaration
when it does.  This would require being sure that we include arpa/inet.h
anywhere we use that function, but there are few enough callers that
that's not much of a hardship.

Alternatively, we could rename our function to pg_inet_net_ntop to
dodge the conflict.  This might be a good idea anyway to avoid
confusion, since our function doesn't necessarily recognize the same
address-family codes that libc would.

* 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.

(The FreeBSD box shows another couple of complaints too, but I think
the fixes for those are uncontroversial.)

Comments?

			regards, tom lane

  2. Re: Residual cpluspluscheck issues

    Jesse Zhang <sbjesse@gmail.com> — 2020-09-30T15:20:45Z

    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

  3. Re: Residual cpluspluscheck issues

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-30T15:47:18Z

    Jesse Zhang <sbjesse@gmail.com> writes:
> 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...

Well, we fixed the case that was discussed at the time [1].

I'm not exactly convinced about removing the register keywords in
s_lock.h.  Those are all associated with asm blocks, which are already
extremely C/GCC specific; complaining that the register declarations
aren't portable seems to be missing the forest for the trees.

BTW, grepping my local tree says that plperl/ppport.h also has some
register variables, which is something we have no control over.

			regards, tom lane

[1] https://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=232720be9