On 2018-09-28 23:22, Kim Barrett wrote:
On Sep 24, 2018, at 4:31 PM, Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com>
wrote:
The second warning about the copy constructor is, for what I can tell, a highly
valid warning and the code it warned on was indeed broken. As far as I can
tell, in a derived copy constructor you should always explicitly initialize the
base class.
I agree the copy constructor warnings are correct and indicate potentially
serious bugs.
These copy constructor changes look correct to me.
The code that is being missed because of this bug is debug-only usage
verification. I think
bad things might happen if we C-heap allocated a ResourceObj and then copied
that object.
Maybe we fortuitously don’t ever do that?
If we had triggered problems we'd probably found out the issue by now,
so its likely we're not doing anything that causes this to happen
currently. But latent bugs like these are scary, and unnecessary,
especially if we can get the compiler's help to avoid them.
It’s unfortunate that the only way to get these warnings from gcc seems to be
via -Wextra.
I agree, it's unfortunate. -Wextra in gcc actually means two things: a
bunch of "extra" warnings, that you can turn on or off as a group only
by enabling or disabling -Wextra, and a set of separate warnings that
this option also turns on by default. The latter is more of a
convenience to get a set of warnings that the gcc authors recommend for
more in-depth warnings. Since they can be individually disabled, we
don't need to care much about them.
In regard to your previous mail, there has not been much change in the
scope of -Wextra. Between gcc 4.8 and 7.3, no changes were made in the
first part (the "extra" warnings), and only four more warnings were
added to the second part (-Wcast-function-type,
-Wimplicit-fallthrough=3, -Wredundant-move and -Wshift-negative-value),
all of which can be turned off if we don't want them.
In general, we try to make the product compile without warnings on
common platforms, but as you say, unless you use one of the compilers
that are regularly used (at Oracle or elsewhere), there's a risk of
triggering new warnings. However, using configure
--disable-warnings-as-errors makes the build pass anyway, and is already
commonly in use by those building on more exotic platforms or compiler
versions.
I would have preferred to have individual warnings to enable, but gcc
has not moved all warnings from -Wextra to individual warnings (new
warnings, though, have all been given individual names). Since the
warnings, as you agree, can find actual bugs, I think it's worth the
hassle to enable -Wextra. (We already do that for all native code in
OpenJDK, except hotspot, btw.)
/Magnus
Bug: https://bugs.openjdk.java.net/browse/JDK-8211073
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8211073-remove-Wno-extra-from-hotspot/webrev.01
/Magnus
[1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
