[chromium-dev] Warnings in third party libraries: why bother?

['Yves Gerey' via Chromium-dev]

Guten Tag,

tl;dr: Why not disabling 'Warning As Errors' on third party code?

Third party libraries usually requires ad-hoc warning suppressions, like these
ones
<https://chromium-review.googlesource.com/c/chromium/deps/nasm/+/1328946/3/BUILD.gn#55>
.
They don't prevent breakage, for instance when we roll Chromium's third
parties into WebRTC, since WebRTC supports more compilers (some older gcc,
MSVC, ...).
Furthermore, I don't see the point of muting warnings manually and
individually, given we don't plan to fix them.

What about simply muting the one appearing via public headers, and leaving
the others? That is, disabling WarningAsError when building the third
parties themselves?

Thank you for your attention!
--
Warm regards,
Yves

[Peter Kasting]

On Wed, Nov 28, 2018 at 12:30 PM 'Yves Gerey' via Chromium-dev <

Post by 'Yves Gerey' via Chromium-dev
Furthermore, I don't see the point of muting warnings manually and
individually, given we don't plan to fix them.

We have fixed many third-party warnings in the past via upstream patches
(speaking from personal experience).

Warnings on third-party code are already at a somewhat lower level than for
Chromium proper; lowering them further increases the risk of real bugs (in
some cases, security bugs) entering the codebase without us being aware.
Since most third-party code is manually-rolled rather than autorolled, the
common case is that we try to update and either things are OK, or we see
new warnings that we fix upstream before doing the roll. These seem like
OK paths to me.

PK

['Reid Kleckner' via Chromium-dev]

I think the important thing is that there should be no warnings visible in
the Chromium build in standard developer configurations. The only
meaningful way to achieve that is to build with -Werror.

If you're using anything other than the tested Chromium toolchains, you
probably want to disable -Werror anyway, which is what it sounds like
you're doing. There's no guarantee that any chromium code (third_party or
not) will be warning free on compilers that aren't being tested upstream.

One thing that might makes sense is to have a less ad-hoc policy for what
warnings chromium cares about in third_party. You could imagine applying
`-w` to all third_party code, or you could pick some subset that we care
about, like MSVC /W3, some GCC -W flags, and leave it at that. The patch
that you point to is disabling four warnings in MSVC's "/W4" category,
which is the most stringent.

Post by Peter Kasting
On Wed, Nov 28, 2018 at 12:30 PM 'Yves Gerey' via Chromium-dev <

Post by 'Yves Gerey' via Chromium-dev
Furthermore, I don't see the point of muting warnings manually and
individually, given we don't plan to fix them.

We have fixed many third-party warnings in the past via upstream patches
(speaking from personal experience).
Warnings on third-party code are already at a somewhat lower level than
for Chromium proper; lowering them further increases the risk of real bugs
(in some cases, security bugs) entering the codebase without us being
aware. Since most third-party code is manually-rolled rather than
autorolled, the common case is that we try to update and either things are
OK, or we see new warnings that we fix upstream before doing the roll.
These seem like OK paths to me.
PK
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFAZdHDRQB37cbsVzmbm_UwK%2BBE_1Tfa%3DEcMm_p0GDK3Bg%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFAZdHDRQB37cbsVzmbm_UwK%2BBE_1Tfa%3DEcMm_p0GDK3Bg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Mike Frysinger]

disabling -Werror is a bad idea imo. there are plenty of warnings which
should be fatal regardless of the source. security, implicit decls, etc...
come to mind as ones that should always be fatal.

after that, it's a debate of what warnings do we want to enable to trade
off pain in fixing vs general code quality. i don't have an opinion there.
-mike

On Wed, Nov 28, 2018 at 12:31 PM 'Yves Gerey' via Chromium-dev <

Post by 'Yves Gerey' via Chromium-dev
Guten Tag,
tl;dr: Why not disabling 'Warning As Errors' on third party code?
Third party libraries usually requires ad-hoc warning suppressions, like these
ones
<https://chromium-review.googlesource.com/c/chromium/deps/nasm/+/1328946/3/BUILD.gn#55>
.
They don't prevent breakage, for instance when we roll Chromium's third
parties into WebRTC, since WebRTC supports more compilers (some older gcc,
MSVC, ...).
Furthermore, I don't see the point of muting warnings manually and
individually, given we don't plan to fix them.
What about simply muting the one appearing via public headers, and leaving
the others? That is, disabling WarningAsError when building the third
parties themselves?
Thank you for your attention!
--
Warm regards,
Yves
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-BmxfBd_KdHTTev1xitUVDDYM9C0w_fyNuhAnW0-EqT1w%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-BmxfBd_KdHTTev1xitUVDDYM9C0w_fyNuhAnW0-EqT1w%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

['Yves Gerey' via Chromium-dev]

I fully agree with the utility of warnings and the greater utility of
addressing them. I was just under the impression that automatically muting
the warnings was already equivalent to disabling -Werror, only more painful.
Glad I was wrong about the 'automatically' part.

Post by Mike Frysinger
disabling -Werror is a bad idea imo. there are plenty of warnings which
should be fatal regardless of the source. security, implicit decls, etc...
come to mind as ones that should always be fatal.
after that, it's a debate of what warnings do we want to enable to trade
off pain in fixing vs general code quality. i don't have an opinion there.
-mike
On Wed, Nov 28, 2018 at 12:31 PM 'Yves Gerey' via Chromium-dev <

Post by 'Yves Gerey' via Chromium-dev
Guten Tag,
tl;dr: Why not disabling 'Warning As Errors' on third party code?
Third party libraries usually requires ad-hoc warning suppressions, like these
ones
<https://chromium-review.googlesource.com/c/chromium/deps/nasm/+/1328946/3/BUILD.gn#55>
.
They don't prevent breakage, for instance when we roll Chromium's third
parties into WebRTC, since WebRTC supports more compilers (some older gcc,
MSVC, ...).
Furthermore, I don't see the point of muting warnings manually and
individually, given we don't plan to fix them.
What about simply muting the one appearing via public headers, and
leaving the others? That is, disabling WarningAsError when building the
third parties themselves?
Thank you for your attention!
--
Warm regards,
Yves
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-BmxfBd_KdHTTev1xitUVDDYM9C0w_fyNuhAnW0-EqT1w%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-BmxfBd_KdHTTev1xitUVDDYM9C0w_fyNuhAnW0-EqT1w%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
Warm regards,
Yves

[Marc-Antoine Ruel]

Here's the rationale to muting warnings:

A few years ago we perf tested the build process and running a build with
warnings was* significantly* slower than a build without any warning.

Probably worth measuring again if you have a few hours to loose.

M-A

Le jeu. 29 nov. 2018, à 03 h 39, 'Yves Gerey' via Chromium-dev <

Post by 'Yves Gerey' via Chromium-dev
I fully agree with the utility of warnings and the greater utility of
addressing them. I was just under the impression that automatically muting
the warnings was already equivalent to disabling -Werror, only more painful.
Glad I was wrong about the 'automatically' part.

Post by Mike Frysinger
disabling -Werror is a bad idea imo. there are plenty of warnings which
should be fatal regardless of the source. security, implicit decls, etc...
come to mind as ones that should always be fatal.
after that, it's a debate of what warnings do we want to enable to trade
off pain in fixing vs general code quality. i don't have an opinion there.
-mike
On Wed, Nov 28, 2018 at 12:31 PM 'Yves Gerey' via Chromium-dev <

Post by 'Yves Gerey' via Chromium-dev
Guten Tag,
tl;dr: Why not disabling 'Warning As Errors' on third party code?
Third party libraries usually requires ad-hoc warning suppressions,
like these ones
<https://chromium-review.googlesource.com/c/chromium/deps/nasm/+/1328946/3/BUILD.gn#55>
.
They don't prevent breakage, for instance when we roll Chromium's third
parties into WebRTC, since WebRTC supports more compilers (some older gcc,
MSVC, ...).
Furthermore, I don't see the point of muting warnings manually and
individually, given we don't plan to fix them.
What about simply muting the one appearing via public headers, and
leaving the others? That is, disabling WarningAsError when building the
third parties themselves?
Thank you for your attention!
--
Warm regards,
Yves
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-BmxfBd_KdHTTev1xitUVDDYM9C0w_fyNuhAnW0-EqT1w%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-BmxfBd_KdHTTev1xitUVDDYM9C0w_fyNuhAnW0-EqT1w%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
Warm regards,
Yves
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-DfxPmxZ13OXDm5iq6otxkKa%3D-nHX_%3D0VpcfDsYTAakGA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-DfxPmxZ13OXDm5iq6otxkKa%3D-nHX_%3D0VpcfDsYTAakGA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Dirk Pranke]

Hm, I don't remember that experiment. I've always understood the rationale
to be more like we want a zero-tolerance policy for warnings in compile
output so that new warnings will actually be noticed and dealt with rather
than ignored (and also we don't want to have to sift through warnings that
are false positives and can be safely ignored). Maybe I am mistaken, though
...

-- Dirk

Post by Marc-Antoine Ruel
A few years ago we perf tested the build process and running a build with
warnings was* significantly* slower than a build without any warning.
Probably worth measuring again if you have a few hours to loose.
M-A
Le jeu. 29 nov. 2018, à 03 h 39, 'Yves Gerey' via Chromium-dev <

Post by 'Yves Gerey' via Chromium-dev
I fully agree with the utility of warnings and the greater utility of
addressing them. I was just under the impression that automatically muting
the warnings was already equivalent to disabling -Werror, only more painful.
Glad I was wrong about the 'automatically' part.

Post by Mike Frysinger
disabling -Werror is a bad idea imo. there are plenty of warnings which
should be fatal regardless of the source. security, implicit decls, etc...
come to mind as ones that should always be fatal.
after that, it's a debate of what warnings do we want to enable to trade
off pain in fixing vs general code quality. i don't have an opinion there.
-mike
On Wed, Nov 28, 2018 at 12:31 PM 'Yves Gerey' via Chromium-dev <

Post by 'Yves Gerey' via Chromium-dev
Guten Tag,
tl;dr: Why not disabling 'Warning As Errors' on third party code?
Third party libraries usually requires ad-hoc warning suppressions,
like these ones
<https://chromium-review.googlesource.com/c/chromium/deps/nasm/+/1328946/3/BUILD.gn#55>
.
They don't prevent breakage, for instance when we roll Chromium's
third parties into WebRTC, since WebRTC supports more compilers (some older
gcc, MSVC, ...).
Furthermore, I don't see the point of muting warnings manually and
individually, given we don't plan to fix them.
What about simply muting the one appearing via public headers, and
leaving the others? That is, disabling WarningAsError when building the
third parties themselves?
Thank you for your attention!
--
Warm regards,
Yves
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-BmxfBd_KdHTTev1xitUVDDYM9C0w_fyNuhAnW0-EqT1w%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-BmxfBd_KdHTTev1xitUVDDYM9C0w_fyNuhAnW0-EqT1w%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
Warm regards,
Yves
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-DfxPmxZ13OXDm5iq6otxkKa%3D-nHX_%3D0VpcfDsYTAakGA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-DfxPmxZ13OXDm5iq6otxkKa%3D-nHX_%3D0VpcfDsYTAakGA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANAQWOUpioPSYsB%2B%2BSMCWkeRVJ82_h5%3Ds-BoT_2FvUZKaaKnAg%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANAQWOUpioPSYsB%2B%2BSMCWkeRVJ82_h5%3Ds-BoT_2FvUZKaaKnAg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

[Marc-Antoine Ruel]

There was definitely a performance aspect to it, at least on MSVC.

M-A

Post by Dirk Pranke
Hm, I don't remember that experiment. I've always understood the rationale
to be more like we want a zero-tolerance policy for warnings in compile
output so that new warnings will actually be noticed and dealt with rather
than ignored (and also we don't want to have to sift through warnings that
are false positives and can be safely ignored). Maybe I am mistaken, though
...
-- Dirk

Post by Marc-Antoine Ruel
A few years ago we perf tested the build process and running a build with
warnings was* significantly* slower than a build without any warning.
Probably worth measuring again if you have a few hours to loose.
M-A
Le jeu. 29 nov. 2018, à 03 h 39, 'Yves Gerey' via Chromium-dev <

Post by 'Yves Gerey' via Chromium-dev
I fully agree with the utility of warnings and the greater utility of
addressing them. I was just under the impression that automatically muting
the warnings was already equivalent to disabling -Werror, only more painful.
Glad I was wrong about the 'automatically' part.

Post by Mike Frysinger
disabling -Werror is a bad idea imo. there are plenty of warnings
which should be fatal regardless of the source. security, implicit decls,
etc... come to mind as ones that should always be fatal.
after that, it's a debate of what warnings do we want to enable to
trade off pain in fixing vs general code quality. i don't have an opinion
there.
-mike
On Wed, Nov 28, 2018 at 12:31 PM 'Yves Gerey' via Chromium-dev <

Post by 'Yves Gerey' via Chromium-dev
Guten Tag,
tl;dr: Why not disabling 'Warning As Errors' on third party code?
Third party libraries usually requires ad-hoc warning suppressions,
like these ones
<https://chromium-review.googlesource.com/c/chromium/deps/nasm/+/1328946/3/BUILD.gn#55>
.
They don't prevent breakage, for instance when we roll Chromium's
third parties into WebRTC, since WebRTC supports more compilers (some older
gcc, MSVC, ...).
Furthermore, I don't see the point of muting warnings manually and
individually, given we don't plan to fix them.
What about simply muting the one appearing via public headers, and
leaving the others? That is, disabling WarningAsError when building the
third parties themselves?
Thank you for your attention!
--
Warm regards,
Yves
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-BmxfBd_KdHTTev1xitUVDDYM9C0w_fyNuhAnW0-EqT1w%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-BmxfBd_KdHTTev1xitUVDDYM9C0w_fyNuhAnW0-EqT1w%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
Warm regards,
Yves
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-DfxPmxZ13OXDm5iq6otxkKa%3D-nHX_%3D0VpcfDsYTAakGA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKdn5-DfxPmxZ13OXDm5iq6otxkKa%3D-nHX_%3D0VpcfDsYTAakGA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.

--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANAQWOUpioPSYsB%2B%2BSMCWkeRVJ82_h5%3Ds-BoT_2FvUZKaaKnAg%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANAQWOUpioPSYsB%2B%2BSMCWkeRVJ82_h5%3Ds-BoT_2FvUZKaaKnAg%40mail.gmail.com?utm_medium=email&utm_source=footer>
.