Improving visibility of compiler warnings
`mach build` attempts to parse compiler warnings to a persisted "database." You can view a list of compiler warnings post build by running `mach warnings-list`. The intent behind this feature was to make it easier to find and fix compiler warnings. After all, something out of sight is out of mind. There have been a few recent changes to increase the visibility of compiler warnings with the expectation being that raising visibility will increase the chances of someone addressing them. After all, a compiler warning is either a) valid and should be fixed or b) invalid and should be ignored. Either way, a compiler warning shouldn't exist. First, `mach build` now prints a list of all parsed compiler warnings at the end of the build. More details are in the commit message: https://hg.mozilla.org/mozilla-central/rev/4abe611696ab Second, Perfherder now records the compiler warning count as a metric. This means we now have alerts when compiler warnings are added or removed, like [1]. And, we can easily see graphs of compiler warnings over time, like [2]. The compiler warnings are also surfaced in the "performance" tab of build jobs on Treeherder, like [3]. The Perfherder alerts and tracking are informational only: nobody will be backing you out merely because you added a compiler warning. While the possibility of doing that now exists, I view that decision as up to the C++ module. I'm not going to advocate for it. So if you feel passionately, take it up with them :) Astute link clickers will note that the count metrics in CI are often noisy - commonly fluctuating by a value of 1-2. I suspect there are race conditions with interleaved process output and/or bugs in the compiler warnings parser/aggregator. Since the values are currently advisory only, I'm very much taking a "perfect is the enemy of good" attitude and have no plans to improve the robustness. [1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700 [2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5 [3] https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=100509284 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
I've actually been meaning to post about this, with some questions. I definitely like that we now print warnings at the end of builds, since it makes them harder to ignore. But the current amount of warnings spew at the end of every build is problematic, especially when a build fails and I need to scroll up several pages to find out why. Can we make some effort to get clean warnings output at the end of standard builds? A huge chunk of the warnings come from NSS and NSPR, and should be easily fixable. Most of the rest seem to come from vendored libraries, and should probably just be whitelisted if we can't get fixes upstreamed. On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote: `mach build` attempts to parse compiler warnings to a persisted "database." You can view a list of compiler warnings post build by running `mach warnings-list`. The intent behind this feature was to make it easier to find and fix compiler warnings. After all, something out of sight is out of mind. There have been a few recent changes to increase the visibility of compiler warnings with the expectation being that raising visibility will increase the chances of someone addressing them. After all, a compiler warning is either a) valid and should be fixed or b) invalid and should be ignored. Either way, a compiler warning shouldn't exist. First, `mach build` now prints a list of all parsed compiler warnings at the end of the build. More details are in the commit message: https://hg.mozilla.org/mozilla-central/rev/4abe611696ab Second, Perfherder now records the compiler warning count as a metric. This means we now have alerts when compiler warnings are added or removed, like [1]. And, we can easily see graphs of compiler warnings over time, like [2]. The compiler warnings are also surfaced in the "performance" tab of build jobs on Treeherder, like [3]. The Perfherder alerts and tracking are informational only: nobody will be backing you out merely because you added a compiler warning. While the possibility of doing that now exists, I view that decision as up to the C++ module. I'm not going to advocate for it. So if you feel passionately, take it up with them :) Astute link clickers will note that the count metrics in CI are often noisy - commonly fluctuating by a value of 1-2. I suspect there are race conditions with interleaved process output and/or bugs in the compiler warnings parser/aggregator. Since the values are currently advisory only, I'm very much taking a "perfect is the enemy of good" attitude and have no plans to improve the robustness. [1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700 [2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5 [3] https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=100509284 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform -- Kris Maglione Senior Firefox Add-ons Engineer Mozilla Corporation The X server has to be the biggest program I've ever seen that doesn't do anything for you. --Ken Thompson ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
I'm not sure if this is exactly the same as the ALLOW_COMPILER_WARNINGS that we use for warnings-on-errors, but FWIW as of a couple of months ago I cleaned out the last warning-allowance in our "own" code. ALLOW_COMPILER_WARNINGS usage is now only in external libraries (I'm counting NSS and NSPR as "external" for this purpose since I can't land code to m-c directly). On Fri, May 19, 2017, at 02:55 PM, Kris Maglione wrote: > I've actually been meaning to post about this, with some questions. > > I definitely like that we now print warnings at the end of builds, since > it > makes them harder to ignore. But the current amount of warnings spew at > the > end of every build is problematic, especially when a build fails and I > need > to scroll up several pages to find out why. > > Can we make some effort to get clean warnings output at the end of > standard > builds? A huge chunk of the warnings come from NSS and NSPR, and should > be > easily fixable. Most of the rest seem to come from vendored libraries, > and > should probably just be whitelisted if we can't get fixes upstreamed. > > On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote: > >`mach build` attempts to parse compiler warnings to a persisted "database." > >You can view a list of compiler warnings post build by running `mach > >warnings-list`. The intent behind this feature was to make it easier to > >find and fix compiler warnings. After all, something out of sight is out of > >mind. > > > >There have been a few recent changes to increase the visibility of compiler > >warnings with the expectation being that raising visibility will increase > >the chances of someone addressing them. After all, a compiler warning is > >either a) valid and should be fixed or b) invalid and should be ignored. > >Either way, a compiler warning shouldn't exist. > > > >First, `mach build` now prints a list of all parsed compiler warnings at > >the end of the build. More details are in the commit message: > >https://hg.mozilla.org/mozilla-central/rev/4abe611696ab > > > >Second, Perfherder now records the compiler warning count as a metric. This > >means we now have alerts when compiler warnings are added or removed, like > >[1]. And, we can easily see graphs of compiler warnings over time, like > >[2]. The compiler warnings are also surfaced in the "performance" tab of > >build jobs on Treeherder, like [3]. > > > >The Perfherder alerts and tracking are informational only: nobody will be > >backing you out merely because you added a compiler warning. While the > >possibility of doing that now exists, I view that decision as up to the C++ > >module. I'm not going to advocate for it. So if you feel passionately, take > >it up with them :) > > > >Astute link clickers will note that the count metrics in CI are often noisy > >- commonly fluctuating by a value of 1-2. I suspect there are race > >conditions with interleaved process output and/or bugs in the compiler > >warnings parser/aggregator. Since the values are currently advisory only, > >I'm very much taking a "perfect is the enemy of good" attitude and have no > >plans to improve the robustness. > > > >[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700 > >[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5 > >[3] > >https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=100509284 > >___ > >dev-platform mailing list > >dev-platform@lists.mozilla.org > >https://lists.mozilla.org/listinfo/dev-platform > > -- > Kris Maglione > Senior Firefox Add-ons Engineer > Mozilla Corporation > > The X server has to be the biggest program I've ever seen that doesn't > do anything for you. > --Ken Thompson > > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On 2017-05-19 2:44 PM, Gregory Szorc wrote: The Perfherder alerts and tracking are informational only: nobody will be backing you out merely because you added a compiler warning. While the possibility of doing that now exists, I view that decision as up to the C++ module. I'm not going to advocate for it. So if you feel passionately, take it up with them :) Always happy to see people find new and unanticipated uses for Perfherder. Should performance sheriffs file bugs when they encounter regressions? e.g. this "alert" https://treeherder.mozilla.org/perf.html#/alerts?id=6673 has the underlying root cause: https://bugzilla.mozilla.org/show_bug.cgi?id=1345368 I worry that filing a bug for every regression is going to be too much. Will ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
It is great to see a good use for compiler warnings and alerts. We have added a lot of data to perfherder and the build metrics are not covered by any sheriffs by default. For these if it is clear who introduced them, we will comment in the bug as a note, but there are no intentions to file bugs for any regressions here. Joel ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
With regard to ALLOW_COMPILER_WARNINGS, I'm strongly of the opinion that we should be providing an option to hide all warnings from modules marked as ALLOW_COMPILER_WARNINGS now, as they are only in "external" libraries which most of us are not working on, and they really clog up my compiler output and make me have to scroll up many many pages in order to see the build errors which are actually my fault. (see this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1301688). Unfortunately, I don't know enough about the build system to write a patch to do this. On Fri, May 19, 2017 at 2:59 PM, David Major wrote: > I'm not sure if this is exactly the same as the ALLOW_COMPILER_WARNINGS > that we use for warnings-on-errors, but FWIW as of a couple of months > ago I cleaned out the last warning-allowance in our "own" code. > ALLOW_COMPILER_WARNINGS usage is now only in external libraries (I'm > counting NSS and NSPR as "external" for this purpose since I can't land > code to m-c directly). > > On Fri, May 19, 2017, at 02:55 PM, Kris Maglione wrote: > > I've actually been meaning to post about this, with some questions. > > > > I definitely like that we now print warnings at the end of builds, since > > it > > makes them harder to ignore. But the current amount of warnings spew at > > the > > end of every build is problematic, especially when a build fails and I > > need > > to scroll up several pages to find out why. > > > > Can we make some effort to get clean warnings output at the end of > > standard > > builds? A huge chunk of the warnings come from NSS and NSPR, and should > > be > > easily fixable. Most of the rest seem to come from vendored libraries, > > and > > should probably just be whitelisted if we can't get fixes upstreamed. > > > > On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote: > > >`mach build` attempts to parse compiler warnings to a persisted > "database." > > >You can view a list of compiler warnings post build by running `mach > > >warnings-list`. The intent behind this feature was to make it easier to > > >find and fix compiler warnings. After all, something out of sight is > out of > > >mind. > > > > > >There have been a few recent changes to increase the visibility of > compiler > > >warnings with the expectation being that raising visibility will > increase > > >the chances of someone addressing them. After all, a compiler warning is > > >either a) valid and should be fixed or b) invalid and should be ignored. > > >Either way, a compiler warning shouldn't exist. > > > > > >First, `mach build` now prints a list of all parsed compiler warnings at > > >the end of the build. More details are in the commit message: > > >https://hg.mozilla.org/mozilla-central/rev/4abe611696ab > > > > > >Second, Perfherder now records the compiler warning count as a metric. > This > > >means we now have alerts when compiler warnings are added or removed, > like > > >[1]. And, we can easily see graphs of compiler warnings over time, like > > >[2]. The compiler warnings are also surfaced in the "performance" tab of > > >build jobs on Treeherder, like [3]. > > > > > >The Perfherder alerts and tracking are informational only: nobody will > be > > >backing you out merely because you added a compiler warning. While the > > >possibility of doing that now exists, I view that decision as up to the > C++ > > >module. I'm not going to advocate for it. So if you feel passionately, > take > > >it up with them :) > > > > > >Astute link clickers will note that the count metrics in CI are often > noisy > > >- commonly fluctuating by a value of 1-2. I suspect there are race > > >conditions with interleaved process output and/or bugs in the compiler > > >warnings parser/aggregator. Since the values are currently advisory > only, > > >I'm very much taking a "perfect is the enemy of good" attitude and have > no > > >plans to improve the robustness. > > > > > >[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700 > > >[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5 > > >[3] > > >https://treeherder.mozilla.org/#/jobs?repo=autoland&; > selectedJob=100509284 > > >___ > > >dev-platform mailing list > > >dev-platform@lists.mozilla.org > > >https://lists.mozilla.org/listinfo/dev-platform > > > > -- > > Kris Maglione > > Senior Firefox Add-ons Engineer > > Mozilla Corporation > > > > The X server has to be the biggest program I've ever seen that doesn't > > do anything for you. > > --Ken Thompson > > > > ___ > > dev-platform mailing list > > dev-platform@lists.mozilla.org > > https://lists.mozilla.org/listinfo/dev-platform > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/list
Re: Improving visibility of compiler warnings
While I agree with that in general, it seems like the warnings in NSS and NSPR, at least, are our responsibility, and should be fixed. I notice the huge number of warnings scrolling by from NSS, in particular, every time I compile, and they make me worry. On Fri, May 19, 2017 at 04:27:47PM -0400, Michael Layzell wrote: With regard to ALLOW_COMPILER_WARNINGS, I'm strongly of the opinion that we should be providing an option to hide all warnings from modules marked as ALLOW_COMPILER_WARNINGS now, as they are only in "external" libraries which most of us are not working on, and they really clog up my compiler output and make me have to scroll up many many pages in order to see the build errors which are actually my fault. (see this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1301688). Unfortunately, I don't know enough about the build system to write a patch to do this. On Fri, May 19, 2017 at 2:59 PM, David Major wrote: I'm not sure if this is exactly the same as the ALLOW_COMPILER_WARNINGS that we use for warnings-on-errors, but FWIW as of a couple of months ago I cleaned out the last warning-allowance in our "own" code. ALLOW_COMPILER_WARNINGS usage is now only in external libraries (I'm counting NSS and NSPR as "external" for this purpose since I can't land code to m-c directly). On Fri, May 19, 2017, at 02:55 PM, Kris Maglione wrote: > I've actually been meaning to post about this, with some questions. > > I definitely like that we now print warnings at the end of builds, since > it > makes them harder to ignore. But the current amount of warnings spew at > the > end of every build is problematic, especially when a build fails and I > need > to scroll up several pages to find out why. > > Can we make some effort to get clean warnings output at the end of > standard > builds? A huge chunk of the warnings come from NSS and NSPR, and should > be > easily fixable. Most of the rest seem to come from vendored libraries, > and > should probably just be whitelisted if we can't get fixes upstreamed. > > On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote: > >`mach build` attempts to parse compiler warnings to a persisted "database." > >You can view a list of compiler warnings post build by running `mach > >warnings-list`. The intent behind this feature was to make it easier to > >find and fix compiler warnings. After all, something out of sight is out of > >mind. > > > >There have been a few recent changes to increase the visibility of compiler > >warnings with the expectation being that raising visibility will increase > >the chances of someone addressing them. After all, a compiler warning is > >either a) valid and should be fixed or b) invalid and should be ignored. > >Either way, a compiler warning shouldn't exist. > > > >First, `mach build` now prints a list of all parsed compiler warnings at > >the end of the build. More details are in the commit message: > >https://hg.mozilla.org/mozilla-central/rev/4abe611696ab > > > >Second, Perfherder now records the compiler warning count as a metric. This > >means we now have alerts when compiler warnings are added or removed, like > >[1]. And, we can easily see graphs of compiler warnings over time, like > >[2]. The compiler warnings are also surfaced in the "performance" tab of > >build jobs on Treeherder, like [3]. > > > >The Perfherder alerts and tracking are informational only: nobody will be > >backing you out merely because you added a compiler warning. While the > >possibility of doing that now exists, I view that decision as up to the C++ > >module. I'm not going to advocate for it. So if you feel passionately, take > >it up with them :) > > > >Astute link clickers will note that the count metrics in CI are often noisy > >- commonly fluctuating by a value of 1-2. I suspect there are race > >conditions with interleaved process output and/or bugs in the compiler > >warnings parser/aggregator. Since the values are currently advisory only, > >I'm very much taking a "perfect is the enemy of good" attitude and have no > >plans to improve the robustness. > > > >[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700 > >[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5 > >[3] > >https://treeherder.mozilla.org/#/jobs?repo=autoland&; selectedJob=100509284 > >___ > >dev-platform mailing list > >dev-platform@lists.mozilla.org > >https://lists.mozilla.org/listinfo/dev-platform > > -- > Kris Maglione > Senior Firefox Add-ons Engineer > Mozilla Corporation > > The X server has to be the biggest program I've ever seen that doesn't > do anything for you. > --Ken Thompson > > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
I strongly agree with you, Michael! Especially now that I'm using IceCC, I pretty much always have to use search-and-replace to find compiler errors. It's a pointless drain on productivity. On Fri, May 19, 2017 at 1:27 PM, Michael Layzell wrote: > With regard to ALLOW_COMPILER_WARNINGS, I'm strongly of the opinion that we > should be providing an option to hide all warnings from modules marked as > ALLOW_COMPILER_WARNINGS now, as they are only in "external" libraries which > most of us are not working on, and they really clog up my compiler output > and make me have to scroll up many many pages in order to see the build > errors which are actually my fault. (see this bug: > https://bugzilla.mozilla.org/show_bug.cgi?id=1301688). Unfortunately, I > don't know enough about the build system to write a patch to do this. > > On Fri, May 19, 2017 at 2:59 PM, David Major wrote: > > > I'm not sure if this is exactly the same as the ALLOW_COMPILER_WARNINGS > > that we use for warnings-on-errors, but FWIW as of a couple of months > > ago I cleaned out the last warning-allowance in our "own" code. > > ALLOW_COMPILER_WARNINGS usage is now only in external libraries (I'm > > counting NSS and NSPR as "external" for this purpose since I can't land > > code to m-c directly). > > > > On Fri, May 19, 2017, at 02:55 PM, Kris Maglione wrote: > > > I've actually been meaning to post about this, with some questions. > > > > > > I definitely like that we now print warnings at the end of builds, > since > > > it > > > makes them harder to ignore. But the current amount of warnings spew at > > > the > > > end of every build is problematic, especially when a build fails and I > > > need > > > to scroll up several pages to find out why. > > > > > > Can we make some effort to get clean warnings output at the end of > > > standard > > > builds? A huge chunk of the warnings come from NSS and NSPR, and should > > > be > > > easily fixable. Most of the rest seem to come from vendored libraries, > > > and > > > should probably just be whitelisted if we can't get fixes upstreamed. > > > > > > On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote: > > > >`mach build` attempts to parse compiler warnings to a persisted > > "database." > > > >You can view a list of compiler warnings post build by running `mach > > > >warnings-list`. The intent behind this feature was to make it easier > to > > > >find and fix compiler warnings. After all, something out of sight is > > out of > > > >mind. > > > > > > > >There have been a few recent changes to increase the visibility of > > compiler > > > >warnings with the expectation being that raising visibility will > > increase > > > >the chances of someone addressing them. After all, a compiler warning > is > > > >either a) valid and should be fixed or b) invalid and should be > ignored. > > > >Either way, a compiler warning shouldn't exist. > > > > > > > >First, `mach build` now prints a list of all parsed compiler warnings > at > > > >the end of the build. More details are in the commit message: > > > >https://hg.mozilla.org/mozilla-central/rev/4abe611696ab > > > > > > > >Second, Perfherder now records the compiler warning count as a metric. > > This > > > >means we now have alerts when compiler warnings are added or removed, > > like > > > >[1]. And, we can easily see graphs of compiler warnings over time, > like > > > >[2]. The compiler warnings are also surfaced in the "performance" tab > of > > > >build jobs on Treeherder, like [3]. > > > > > > > >The Perfherder alerts and tracking are informational only: nobody will > > be > > > >backing you out merely because you added a compiler warning. While the > > > >possibility of doing that now exists, I view that decision as up to > the > > C++ > > > >module. I'm not going to advocate for it. So if you feel passionately, > > take > > > >it up with them :) > > > > > > > >Astute link clickers will note that the count metrics in CI are often > > noisy > > > >- commonly fluctuating by a value of 1-2. I suspect there are race > > > >conditions with interleaved process output and/or bugs in the compiler > > > >warnings parser/aggregator. Since the values are currently advisory > > only, > > > >I'm very much taking a "perfect is the enemy of good" attitude and > have > > no > > > >plans to improve the robustness. > > > > > > > >[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700 > > > >[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5 > > > >[3] > > > >https://treeherder.mozilla.org/#/jobs?repo=autoland&; > > selectedJob=100509284 > > > >___ > > > >dev-platform mailing list > > > >dev-platform@lists.mozilla.org > > > >https://lists.mozilla.org/listinfo/dev-platform > > > > > > -- > > > Kris Maglione > > > Senior Firefox Add-ons Engineer > > > Mozilla Corporation > > > > > > The X server has to be the biggest program I've ever seen that doesn't > > > do anything for you. > > > --Ken Thompson
Re: Improving visibility of compiler warnings
On Fri, May 19, 2017 at 1:27 PM, Michael Layzell wrote: > With regard to ALLOW_COMPILER_WARNINGS, I'm strongly of the opinion that we > should be providing an option to hide all warnings from modules marked as > ALLOW_COMPILER_WARNINGS now, as they are only in "external" libraries which > most of us are not working on, and they really clog up my compiler output > and make me have to scroll up many many pages in order to see the build > errors which are actually my fault. (see this bug: > https://bugzilla.mozilla.org/show_bug.cgi?id=1301688). Unfortunately, I > don't know enough about the build system to write a patch to do this. > I like to teach others how to fish: https://dxr.mozilla.org/mozilla-central/search?q=path%3Amoz.build+C&redirect=false Code to call into external build systems is typically in config/external/. See e.g. config/external/icu/defs.mozbuild for where compiler flags for ICU are defined. > > On Fri, May 19, 2017 at 2:59 PM, David Major wrote: > > > I'm not sure if this is exactly the same as the ALLOW_COMPILER_WARNINGS > > that we use for warnings-on-errors, but FWIW as of a couple of months > > ago I cleaned out the last warning-allowance in our "own" code. > > ALLOW_COMPILER_WARNINGS usage is now only in external libraries (I'm > > counting NSS and NSPR as "external" for this purpose since I can't land > > code to m-c directly). > > > > On Fri, May 19, 2017, at 02:55 PM, Kris Maglione wrote: > > > I've actually been meaning to post about this, with some questions. > > > > > > I definitely like that we now print warnings at the end of builds, > since > > > it > > > makes them harder to ignore. But the current amount of warnings spew at > > > the > > > end of every build is problematic, especially when a build fails and I > > > need > > > to scroll up several pages to find out why. > > > > > > Can we make some effort to get clean warnings output at the end of > > > standard > > > builds? A huge chunk of the warnings come from NSS and NSPR, and should > > > be > > > easily fixable. Most of the rest seem to come from vendored libraries, > > > and > > > should probably just be whitelisted if we can't get fixes upstreamed. > > > > > > On Fri, May 19, 2017 at 11:44:08AM -0700, Gregory Szorc wrote: > > > >`mach build` attempts to parse compiler warnings to a persisted > > "database." > > > >You can view a list of compiler warnings post build by running `mach > > > >warnings-list`. The intent behind this feature was to make it easier > to > > > >find and fix compiler warnings. After all, something out of sight is > > out of > > > >mind. > > > > > > > >There have been a few recent changes to increase the visibility of > > compiler > > > >warnings with the expectation being that raising visibility will > > increase > > > >the chances of someone addressing them. After all, a compiler warning > is > > > >either a) valid and should be fixed or b) invalid and should be > ignored. > > > >Either way, a compiler warning shouldn't exist. > > > > > > > >First, `mach build` now prints a list of all parsed compiler warnings > at > > > >the end of the build. More details are in the commit message: > > > >https://hg.mozilla.org/mozilla-central/rev/4abe611696ab > > > > > > > >Second, Perfherder now records the compiler warning count as a metric. > > This > > > >means we now have alerts when compiler warnings are added or removed, > > like > > > >[1]. And, we can easily see graphs of compiler warnings over time, > like > > > >[2]. The compiler warnings are also surfaced in the "performance" tab > of > > > >build jobs on Treeherder, like [3]. > > > > > > > >The Perfherder alerts and tracking are informational only: nobody will > > be > > > >backing you out merely because you added a compiler warning. While the > > > >possibility of doing that now exists, I view that decision as up to > the > > C++ > > > >module. I'm not going to advocate for it. So if you feel passionately, > > take > > > >it up with them :) > > > > > > > >Astute link clickers will note that the count metrics in CI are often > > noisy > > > >- commonly fluctuating by a value of 1-2. I suspect there are race > > > >conditions with interleaved process output and/or bugs in the compiler > > > >warnings parser/aggregator. Since the values are currently advisory > > only, > > > >I'm very much taking a "perfect is the enemy of good" attitude and > have > > no > > > >plans to improve the robustness. > > > > > > > >[1] https://treeherder.mozilla.org/perf.html#/alerts?id=6700 > > > >[2] https://mzl.la/2qFN0me and https://mzl.la/2rAkWR5 > > > >[3] > > > >https://treeherder.mozilla.org/#/jobs?repo=autoland&; > > selectedJob=100509284 > > > >___ > > > >dev-platform mailing list > > > >dev-platform@lists.mozilla.org > > > >https://lists.mozilla.org/listinfo/dev-platform > > > > > > -- > > > Kris Maglione > > > Senior Firefox Add-ons Engineer > > > Mozilla Corporation > > > > > > The X server has to be th
Re: Improving visibility of compiler warnings
On Sat, May 20, 2017 at 4:55 AM, Kris Maglione wrote: > Can we make some effort to get clean warnings output at the end of standard > builds? A huge chunk of the warnings come from NSS and NSPR, and should be > easily fixable. Hmm, these are all -Wsign-compare issues bar one, which is fixed upstream. We have an open bug tracking the warnings (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS). NSS is built with -Werror separately, but disables errors on -Wsign-compare. Disabling those warnings for a Firefox build of NSS wouldn't be so bad now that we share gyp config. Based on a recent build, that's 139 messages (add 36 if you want to add nspr). I've spent a little time looking into the real issues. Fixing requires some care, since it touches ABI compat in many places. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On Sat, May 20, 2017 at 08:36:13PM +1000, Martin Thomson wrote: On Sat, May 20, 2017 at 4:55 AM, Kris Maglione wrote: Can we make some effort to get clean warnings output at the end of standard builds? A huge chunk of the warnings come from NSS and NSPR, and should be easily fixable. Hmm, these are all -Wsign-compare issues bar one, which is fixed upstream. We have an open bug tracking the warnings (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS). NSS is built with -Werror separately, but disables errors on -Wsign-compare. Disabling those warnings for a Firefox build of NSS wouldn't be so bad now that we share gyp config. Based on a recent build, that's 139 messages (add 36 if you want to add nspr). Is there a particularly good reason that NSS needs to disable -Wsign-compare? That seems like a footgun waiting to happen, especially in crypto code. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On Sat, May 20, 2017 at 1:16 PM, Kris Maglione wrote: > On Sat, May 20, 2017 at 08:36:13PM +1000, Martin Thomson wrote: > >> On Sat, May 20, 2017 at 4:55 AM, Kris Maglione >> wrote: >> >>> Can we make some effort to get clean warnings output at the end of >>> standard >>> builds? A huge chunk of the warnings come from NSS and NSPR, and should >>> be >>> easily fixable. >>> >> >> Hmm, these are all -Wsign-compare issues bar one, which is fixed >> upstream. We have an open bug tracking the warnings >> (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically >> https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS). >> >> NSS is built with -Werror separately, but disables errors on >> -Wsign-compare. Disabling those warnings for a Firefox build of NSS >> wouldn't be so bad now that we share gyp config. Based on a recent >> build, that's 139 messages (add 36 if you want to add nspr). >> > > Is there a particularly good reason that NSS needs to disable > -Wsign-compare? That seems like a footgun waiting to happen, especially in > crypto code. Like many other pieces of old code, there are a lot of things that trigger compiler warnings which we have been progressively removing, but we haven't removed these yet. Ideally we would have some tooling that would distinguish new warnings from old warnings, but absent that, until we fix all the existing warnings it's either disable -Werror or disable this particular warning. It's not something we're particularly happy about. -Ekr > > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
Hi, On 2017/05/20 19:36, Martin Thomson wrote: On Sat, May 20, 2017 at 4:55 AM, Kris Maglione wrote: Can we make some effort to get clean warnings output at the end of standard builds? A huge chunk of the warnings come from NSS and NSPR, and should be easily fixable. Hmm, these are all -Wsign-compare issues bar one, which is fixed upstream. We have an open bug tracking the warnings (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS). I am surprised that https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 was mentioned and when I looked at it, it was filed by me. :-) The patches there were filed about four months ago, and recently I noticed that I need to add a few more changes presumably due to code changes, AND I found that I must have disabled a few modules from compiling to build C-C TB, and a full M-C FF build with -Wsign-compare and warning as error setting needs still a few more changes (not that many). I will upload the latest patches later. I agree that disabling this check is a little uncomfortable for crypto code. My patch is pasting over the mismatched comparison by casting one type to the other. In most of the cases, I see that the value should not go that high for wraparound near 2^31, etc. but still I would have inserted an assert() to detect a problematic situation if I have infinite amount of time to spend reading the code. My patches would alert astute coders to look at the places where the problem may arise and insert assert() appropriately. TIA NSS is built with -Werror separately, but disables errors on -Wsign-compare. Disabling those warnings for a Firefox build of NSS wouldn't be so bad now that we share gyp config. Based on a recent build, that's 139 messages (add 36 if you want to add nspr). I've spent a little time looking into the real issues. Fixing requires some care, since it touches ABI compat in many places. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On Sat, 20 May 2017 14:59:11 -0400, Eric Rescorla wrote: > On Sat, May 20, 2017 at 1:16 PM, Kris Maglione > wrote: > >> On Sat, May 20, 2017 at 08:36:13PM +1000, Martin Thomson wrote: >> >>> Hmm, these are all -Wsign-compare issues bar one, which is fixed >>> upstream. We have an open bug tracking the warnings >>> (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically >>> https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS). >>> >>> NSS is built with -Werror separately, but disables errors on >>> -Wsign-compare. Disabling those warnings for a Firefox build of NSS >>> wouldn't be so bad now that we share gyp config. Based on a recent >>> build, that's 139 messages (add 36 if you want to add nspr). >>> >> >> Is there a particularly good reason that NSS needs to disable >> -Wsign-compare? That seems like a footgun waiting to happen, especially in >> crypto code. > > > Like many other pieces of old code, there are a lot of things that trigger > compiler warnings which we have been progressively removing, but we > haven't removed these yet. Ideally we would have some tooling that > would distinguish new warnings from old warnings, but absent that, > until we fix all the existing warnings it's either disable -Werror or > disable this particular warning. It's not something we're particularly > happy about. -Wno-error=sign-compare is also an option, but a wall of sign-compare warnings may not be helpful in general. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
I don't have enough time to work on a proper solution, but I wrote a simple rust wrapper which just consumes stderr from the wrapped process, and doesn't write it out unless the internal command exited with a non-zero exit code. I figured I'd post it in case anyone else finds it useful. https://github.com/mystor/nowarn Very hacky, but it helps clean up the spam of compiler warnings to let me actually find my build errors. On Mon, May 22, 2017 at 7:16 PM, Karl Tomlinson wrote: > On Sat, 20 May 2017 14:59:11 -0400, Eric Rescorla wrote: > > > On Sat, May 20, 2017 at 1:16 PM, Kris Maglione > > wrote: > > > >> On Sat, May 20, 2017 at 08:36:13PM +1000, Martin Thomson wrote: > >> > >>> Hmm, these are all -Wsign-compare issues bar one, which is fixed > >>> upstream. We have an open bug tracking the warnings > >>> (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically > >>> https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS). > >>> > >>> NSS is built with -Werror separately, but disables errors on > >>> -Wsign-compare. Disabling those warnings for a Firefox build of NSS > >>> wouldn't be so bad now that we share gyp config. Based on a recent > >>> build, that's 139 messages (add 36 if you want to add nspr). > >>> > >> > >> Is there a particularly good reason that NSS needs to disable > >> -Wsign-compare? That seems like a footgun waiting to happen, especially > in > >> crypto code. > > > > > > Like many other pieces of old code, there are a lot of things that > trigger > > compiler warnings which we have been progressively removing, but we > > haven't removed these yet. Ideally we would have some tooling that > > would distinguish new warnings from old warnings, but absent that, > > until we fix all the existing warnings it's either disable -Werror or > > disable this particular warning. It's not something we're particularly > > happy about. > > -Wno-error=sign-compare is also an option, > but a wall of sign-compare warnings may not be helpful in general. > > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
I had it pointed out to me on IRC by jcristau that `chronic` already exists as a Unix utility from moreutils: https://manpages.debian.org/jessie/moreutils/chronic.1.en.html It does pretty much the exact same thing as nowarn, but is probably better written ^.^. On Tue, May 23, 2017 at 11:56 AM, Michael Layzell wrote: > I don't have enough time to work on a proper solution, but I wrote a > simple rust wrapper which just consumes stderr from the wrapped process, > and doesn't write it out unless the internal command exited with a non-zero > exit code. I figured I'd post it in case anyone else finds it useful. > https://github.com/mystor/nowarn > > Very hacky, but it helps clean up the spam of compiler warnings to let me > actually find my build errors. > > On Mon, May 22, 2017 at 7:16 PM, Karl Tomlinson wrote: > >> On Sat, 20 May 2017 14:59:11 -0400, Eric Rescorla wrote: >> >> > On Sat, May 20, 2017 at 1:16 PM, Kris Maglione >> > wrote: >> > >> >> On Sat, May 20, 2017 at 08:36:13PM +1000, Martin Thomson wrote: >> >> >> >>> Hmm, these are all -Wsign-compare issues bar one, which is fixed >> >>> upstream. We have an open bug tracking the warnings >> >>> (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and >> specifically >> >>> https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS). >> >>> >> >>> NSS is built with -Werror separately, but disables errors on >> >>> -Wsign-compare. Disabling those warnings for a Firefox build of NSS >> >>> wouldn't be so bad now that we share gyp config. Based on a recent >> >>> build, that's 139 messages (add 36 if you want to add nspr). >> >>> >> >> >> >> Is there a particularly good reason that NSS needs to disable >> >> -Wsign-compare? That seems like a footgun waiting to happen, >> especially in >> >> crypto code. >> > >> > >> > Like many other pieces of old code, there are a lot of things that >> trigger >> > compiler warnings which we have been progressively removing, but we >> > haven't removed these yet. Ideally we would have some tooling that >> > would distinguish new warnings from old warnings, but absent that, >> > until we fix all the existing warnings it's either disable -Werror or >> > disable this particular warning. It's not something we're particularly >> > happy about. >> >> -Wno-error=sign-compare is also an option, >> but a wall of sign-compare warnings may not be helpful in general. >> >> ___ >> dev-platform mailing list >> dev-platform@lists.mozilla.org >> https://lists.mozilla.org/listinfo/dev-platform >> > > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On Sat, May 20, 2017 at 6:36 AM, Martin Thomson wrote: > Hmm, these are all -Wsign-compare issues bar one, which is fixed > upstream. We have an open bug tracking the warnings > (https://bugzilla.mozilla.org/show_bug.cgi?id=1307958 and specifically > https://bugzilla.mozilla.org/show_bug.cgi?id=1212199 for NSS). > > NSS is built with -Werror separately, but disables errors on > -Wsign-compare. Disabling those warnings for a Firefox build of NSS > wouldn't be so bad now that we share gyp config. Based on a recent > build, that's 139 messages (add 36 if you want to add nspr). Where does NSS do this? Cloning the NSS tree and grepping for sign-compare turns up no disabling of -Wsign-compare, except perhaps in XCode for gtest itself. Thanks, -Nathan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On Thu, May 25, 2017 at 6:03 AM, Nathan Froyd wrote: > Where does NSS do this? Cloning the NSS tree and grepping for > sign-compare turns up no disabling of -Wsign-compare, except perhaps > in XCode for gtest itself. My bad, -Wsign-compare is in -Wextra, and we don't enable anything extra. We disable errors for C4018 on MSVC, which is what I was thinking of. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On 05/19/2017 02:44 PM, Gregory Szorc wrote: `mach build` attempts to parse compiler warnings to a persisted "database." You can view a list of compiler warnings post build by running `mach warnings-list`. The intent behind this feature was to make it easier to find and fix compiler warnings. After all, something out of sight is out of mind. There have been a few recent changes to increase the visibility of compiler warnings with the expectation being that raising visibility will increase the chances of someone addressing them. After all, a compiler warning is either a) valid and should be fixed or b) invalid and should be ignored. Either way, a compiler warning shouldn't exist. Since mystor and billm's objection seems to have gone unaddressed in this thread, let me try once again. There is a (c) scenario that you are ignoring here: c) the warning is valid and should be fixed but it's not more important than other things than the developer may want to do at the moment. I would like to posit that this is often the case. Let's look at what I get these days at the end of a normal debug build on Linux. This output is long and I am intentionally pasting the full thing here in order to make a point on how much useless information we are presenting at the end of each build: 18:39.34 warning: db/sqlite3/src/sqlite3.c:131236:39 [-Wunreachable-code] code will never be executed 18:39.34 warning: db/sqlite3/src/sqlite3.c:131292:39 [-Wunreachable-code] code will never be executed 18:39.34 warning: db/sqlite3/src/sqlite3.c:137670:9 [-Wunreachable-code] code will never be executed 18:39.34 warning: gfx/angle/src/compiler/translator/ASTMetadataHLSL.cpp:93:15 [-Wimplicit-fallthrough] unannotated fall-through between switch labels 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:1123:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:3640:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:3808:9 [-Wimplicit-fallthrough] unannotated fall-through between switch labels 18:39.34 warning: gfx/cairo/libpixman/src/pixman-bits-image.c:268:32 [-Wshift-negative-value] shifting a negative signed value is undefined 18:39.34 warning: gfx/cairo/libpixman/src/pixman-linear-gradient.c:395:6 [-Wunreachable-code] code will never be executed 18:39.34 warning: gfx/cairo/libpixman/src/pixman-x86.c:80:5 [-Wexpansion-to-defined] macro expansion producing 'defined' has undefined behavior 18:39.34 warning: gfx/cairo/libpixman/src/pixman-x86.c:119:5 [-Wexpansion-to-defined] macro expansion producing 'defined' has undefined behavior 18:39.34 warning: intl/hyphenation/hyphen/hyphen.c:332:27 [-Wsign-compare] comparison of integers of different signs: 'int' and 'unsigned long' 18:39.34 warning: intl/icu/source/common/locdspnm.cpp:286:14 [-Wunused-private-field] private field 'capitalizationBrkIter' is not used 18:39.34 warning: intl/icu/source/common/udataswp.c:438:29 [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka 'int') and 'unsigned long' 18:39.34 warning: intl/icu/source/common/ulist.c:161:24 [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka 'int') and 'unsigned long' 18:39.34 warning: intl/icu/source/common/uloc_tag.c:1374:31 [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka 'int') and 'unsigned long' 18:39.34 warning: intl/icu/source/common/uloc_tag.c:1409:36 [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka 'int') and 'unsigned long' 18:39.34 warning: intl/icu/source/common/ures_cnv.c:46:18 [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka 'int') and 'unsigned long' 18:39.34 warning: intl/icu/source/common/ures_cnv.c:64:22 [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka 'int') and 'unsigned long' 18:39.34 warning: intl/icu/source/common/ushape.cpp:1247:12 [-Wcomma] possible misuse of comma operator here 18:39.34 warning: intl/icu/source/common/ustring.cpp:860:57 [-Wcomma] possible misuse of comma operator here 18:39.34 warning: intl/icu/source/common/ustring.cpp:870:57 [-Wcomma] possible misuse of comma operator here 18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:488:47 [-Wcomma] possible misuse of comma operator here 18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:535:47 [-Wcomma] possible misuse of comma operator here 18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:609:51 [-Wcomma] possible misuse of comma operator here 18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:655:47 [-Wcomma] possible misuse of comma operator here 18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:704:47 [-Wcomma] possible misuse of comma operator here 18:39.34 warning: intl/icu/source/common/utrace.c:149:16 [-Wsign-compare] compa
Re: Improving visibility of compiler warnings
On 5/25/17 8:31 AM, Ehsan Akhgari wrote: This currently only serves to make it more difficult to find compiler errors when they occur. Fwiw (and not to distract from your main point), https://bugzilla.mozilla.org/show_bug.cgi?id=1367405 just got fixed so we should no longer get the warning spew when there are compiler errors. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
I'd like to second Ehsan's point, but also expand upon it into a more general observation. As it becomes progressively more difficult to build Firefox without mach, it becomes increasingly important that mach respect people's workflows. For those of us who were comfortable with make and the behavior of having relatively unfiltered access to what the build system is doing, it would be great if mach could have some set of flags to preserve that view (cf. the flags to remove the timestamps so that emacs compiler mode works). -Ekr ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On 2017-05-25 5:31 AM, Ehsan Akhgari wrote: On 05/19/2017 02:44 PM, Gregory Szorc wrote: `mach build` attempts to parse compiler warnings to a persisted "database." You can view a list of compiler warnings post build by running `mach warnings-list`. The intent behind this feature was to make it easier to find and fix compiler warnings. After all, something out of sight is out of mind. Given that we treat warnings as errors on CI and most directories that opt out of warnings-as-errors are third-party code (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to make the warning list any more visible. A warning regression in nearly all first-party code is already treated as an error. [1] https://searchfox.org/mozilla-central/search?q=ALLOW_COMPILER_WARNINGS&case=true&path=moz.build ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On Thu, May 25, 2017 at 08:31:24AM -0400, Ehsan Akhgari wrote: What was the motivation behind this change? Was there a complaint from a significant number of developers about it being difficult fixing compiler warnings grepping for things like "warning:" or using ./mach warnings-list? Was the feedback from developers who have the use case of writing code and fix compiler errors in such code taken into account? For what it's worth, I've often wished for something like this when watching huge numbers of warnings flow by in NSS and media code, if only to motivate people to fix them. But the fact that we compile with -Werror and generally only get warnings in third-party code (ignoring NSS and NSPR) means that they're effectively pretty useless, and we'd be better off just disabling warnings in that code. What I really want at the end of a compile is a list of warnings and errors that matter, rather than a lot of useless warning spew that makes it harder to find the ones that do among the ones that don't. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote: > On 2017-05-25 5:31 AM, Ehsan Akhgari wrote: > > On 05/19/2017 02:44 PM, Gregory Szorc wrote: > >> `mach build` attempts to parse compiler warnings to a persisted > >> "database." > >> You can view a list of compiler warnings post build by running `mach > >> warnings-list`. The intent behind this feature was to make it easier to > >> find and fix compiler warnings. After all, something out of sight is > >> out of > >> mind. > > Given that we treat warnings as errors on CI and most directories that > opt out of warnings-as-errors are third-party code > (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to > make the warning list any more visible. A warning regression in nearly > all first-party code is already treated as an error. > > > [1] > https://searchfox.org/mozilla-central/search?q=ALLOW_COMPILER_WARNINGS&case=true&path=moz.build Tangentially: "we treat warnings as errors on CI" -- It'd be great if local builds could have that behavior by default, so we (or just I?) don't get caught by surprise when warnings-as-errors appear after landing. Or at least: Can I opt-in locally? (I tried 'ac_add_options --enable-warnings-ar-errors' in mozconfig, but that was not recognized.) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On Thu, May 25, 2017 at 4:03 PM, wrote: > On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote: > > On 2017-05-25 5:31 AM, Ehsan Akhgari wrote: > > > On 05/19/2017 02:44 PM, Gregory Szorc wrote: > > >> `mach build` attempts to parse compiler warnings to a persisted > > >> "database." > > >> You can view a list of compiler warnings post build by running `mach > > >> warnings-list`. The intent behind this feature was to make it easier > to > > >> find and fix compiler warnings. After all, something out of sight is > > >> out of > > >> mind. > > > > Given that we treat warnings as errors on CI and most directories that > > opt out of warnings-as-errors are third-party code > > (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to > > make the warning list any more visible. A warning regression in nearly > > all first-party code is already treated as an error. > > > > > > [1] > > https://searchfox.org/mozilla-central/search?q=ALLOW_ > COMPILER_WARNINGS&case=true&path=moz.build > > Tangentially: "we treat warnings as errors on CI" -- It'd be great if > local builds could have that behavior by default, so we (or just I?) don't > get caught by surprise when warnings-as-errors appear after landing. > > Or at least: Can I opt-in locally? (I tried 'ac_add_options > --enable-warnings-ar-errors' in mozconfig, but that was not recognized.) > This has worked for me before: ac_add_options --enable-warnings-as-errors ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
I think we disable it for local builds because we don't control what versions of tools folks use. So clang vFOO might spit out errors we don't see in clang vBAR and it would be a huge pain if those failed locally even though they'd be fine in automation. -e On Thu, May 25, 2017 at 4:07 PM, Andrew McCreight wrote: > On Thu, May 25, 2017 at 4:03 PM, wrote: > > > On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote: > > > On 2017-05-25 5:31 AM, Ehsan Akhgari wrote: > > > > On 05/19/2017 02:44 PM, Gregory Szorc wrote: > > > >> `mach build` attempts to parse compiler warnings to a persisted > > > >> "database." > > > >> You can view a list of compiler warnings post build by running `mach > > > >> warnings-list`. The intent behind this feature was to make it easier > > to > > > >> find and fix compiler warnings. After all, something out of sight is > > > >> out of > > > >> mind. > > > > > > Given that we treat warnings as errors on CI and most directories that > > > opt out of warnings-as-errors are third-party code > > > (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to > > > make the warning list any more visible. A warning regression in nearly > > > all first-party code is already treated as an error. > > > > > > > > > [1] > > > https://searchfox.org/mozilla-central/search?q=ALLOW_ > > COMPILER_WARNINGS&case=true&path=moz.build > > > > Tangentially: "we treat warnings as errors on CI" -- It'd be great if > > local builds could have that behavior by default, so we (or just I?) > don't > > get caught by surprise when warnings-as-errors appear after landing. > > > > Or at least: Can I opt-in locally? (I tried 'ac_add_options > > --enable-warnings-ar-errors' in mozconfig, but that was not recognized.) > > > > This has worked for me before: > ac_add_options --enable-warnings-as-errors > > > ___ > > dev-platform mailing list > > dev-platform@lists.mozilla.org > > https://lists.mozilla.org/listinfo/dev-platform > > > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On Friday, May 26, 2017 at 11:08:09 AM UTC+12, Andrew McCreight wrote: > On Thu, May 25, 2017 at 4:03 PM, wrote: > > > On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote: > > > On 2017-05-25 5:31 AM, Ehsan Akhgari wrote: > > > > On 05/19/2017 02:44 PM, Gregory Szorc wrote: > > > >> `mach build` attempts to parse compiler warnings to a persisted > > > >> "database." > > > >> You can view a list of compiler warnings post build by running `mach > > > >> warnings-list`. The intent behind this feature was to make it easier > > to > > > >> find and fix compiler warnings. After all, something out of sight is > > > >> out of > > > >> mind. > > > > > > Given that we treat warnings as errors on CI and most directories that > > > opt out of warnings-as-errors are third-party code > > > (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to > > > make the warning list any more visible. A warning regression in nearly > > > all first-party code is already treated as an error. > > > > > > > > > [1] > > > https://searchfox.org/mozilla-central/search?q=ALLOW_ > > COMPILER_WARNINGS&case=true&path=moz.build > > > > Tangentially: "we treat warnings as errors on CI" -- It'd be great if > > local builds could have that behavior by default, so we (or just I?) don't > > get caught by surprise when warnings-as-errors appear after landing. > > > > Or at least: Can I opt-in locally? (I tried 'ac_add_options > > --enable-warnings-ar-errors' in mozconfig, but that was not recognized.) > > > > This has worked for me before: > ac_add_options --enable-warnings-as-errors Haha, I've just noticed that I had typed '--enable-warnings-ar-errors' (Notice the 'ar' instead of 'as' -- must have been pirate day!) Thanks Andrew. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On Thu, May 25, 2017 at 4:11 PM, Eric Rahm wrote: > I think we disable it for local builds because we don't control what > versions of tools folks use. So clang vFOO might spit out errors we don't > see in clang vBAR and it would be a huge pain if those failed locally even > though they'd be fine in automation. > Exactly. When we offer a build mode that uses the exact toolchain that automation is using and can guarantee that the local environment reasonably approximates CI, we can enable --enable-warnings-as-errors by default in that configuration. We'll likely need a flag on `mach build` to disable that warnings as errors per invocation, as sometimes developers just want to get code to compile, even if there are warnings. But the default should be to match CI so there are fewer surprises and errors are caught quicker. Of course, the more we write Rust the more warnings as errors becomes the explicit behavior because Rust just flat out refuses to compile problems that C++ compilers treat as warnings. > > On Thu, May 25, 2017 at 4:07 PM, Andrew McCreight > wrote: > > > On Thu, May 25, 2017 at 4:03 PM, wrote: > > > > > On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote: > > > > On 2017-05-25 5:31 AM, Ehsan Akhgari wrote: > > > > > On 05/19/2017 02:44 PM, Gregory Szorc wrote: > > > > >> `mach build` attempts to parse compiler warnings to a persisted > > > > >> "database." > > > > >> You can view a list of compiler warnings post build by running > `mach > > > > >> warnings-list`. The intent behind this feature was to make it > easier > > > to > > > > >> find and fix compiler warnings. After all, something out of sight > is > > > > >> out of > > > > >> mind. > > > > > > > > Given that we treat warnings as errors on CI and most directories > that > > > > opt out of warnings-as-errors are third-party code > > > > (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to > > > > make the warning list any more visible. A warning regression in > nearly > > > > all first-party code is already treated as an error. > > > > > > > > > > > > [1] > > > > https://searchfox.org/mozilla-central/search?q=ALLOW_ > > > COMPILER_WARNINGS&case=true&path=moz.build > > > > > > Tangentially: "we treat warnings as errors on CI" -- It'd be great if > > > local builds could have that behavior by default, so we (or just I?) > > don't > > > get caught by surprise when warnings-as-errors appear after landing. > > > > > > Or at least: Can I opt-in locally? (I tried 'ac_add_options > > > --enable-warnings-ar-errors' in mozconfig, but that was not > recognized.) > > > > > > > This has worked for me before: > > ac_add_options --enable-warnings-as-errors > > > > > > ___ > > > dev-platform mailing list > > > dev-platform@lists.mozilla.org > > > https://lists.mozilla.org/listinfo/dev-platform > > > > > ___ > > dev-platform mailing list > > dev-platform@lists.mozilla.org > > https://lists.mozilla.org/listinfo/dev-platform > > > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On 05/25/2017 02:14 PM, Gregory Szorc wrote: On Thu, May 25, 2017 at 5:31 AM, Ehsan Akhgari mailto:ehsan.akhg...@gmail.com>> wrote: What was the motivation behind this change? Was there a complaint from a significant number of developers about it being difficult fixing compiler warnings grepping for things like "warning:" or using ./mach warnings-list? Was the feedback from developers who have the use case of writing code and fix compiler errors in such code taken into account? The motification was that relevant compiler warnings are flying under the radar and are detected too late in the development process - such as when pushing to Try. We want to catch problems as early in the development cycle as possible to reduce the end-to-end times for patch development. There have been complaints from developers over the years about this. What specifically motivated me to make this change recently was finding a pattern of feature requests during bug triage that all seemed to point to improving visibility of compiler warnings as a potential solution. I fully acknowledge that the spew of warnings at the end of the build is annoying. This was expected to cause temporary pain. The hope was that this would spur people into fixing the warnings. FWIW, IMO this is not OK. I don't think there is any consensus that fixing compiler warnings is suddenly of such a great importance that we need to interrupt everyone's different workflows without them having opted into doing the work to fix the warnings. And it may very well turn out that a project with a different format that isn't structured around the tools forcing people into fixing things may be more suitable to efforts like this, as years of work on https://bugzilla.mozilla.org/show_bug.cgi?id=buildwarning shows. There has been some movement there. ICU warnings were suppressed, for example. And, there has been talk in this thread (and presumably elsewhere) about fixing other large offenders. So on that front, I'd say this change was successful. I hope in the future you would consider discussing disruptive changes like this first instead of announcing them post-facto. Maybe at least we would have been able to think of a better initial implementation including not emitting the list for third-party library warnings or failed builds. Or maybe we should have allowed people to opt in first. As the bug above shows, there are people who are interested to help in this area. But the pace of progress hasn't been terrific and the status quo is annoying. This thread is also sapping up people's time. I've prepared a patch to tweak the behavior that I think strikes a compromise. I'll file a bug to track the review shortly. Thank you. I think most of the immediate productivity drain issues are fixed for now. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On 5/25/17 6:11 PM, Eric Rahm wrote: I think we disable it for local builds because we don't control what versions of tools folks use. So clang vFOO might spit out errors we don't see in clang vBAR and it would be a huge pain if those failed locally even though they'd be fine in automation. It should be possible to check the compiler and version and enable it by default if it's the same version as the ones on our check-in infrastructure. -- Joshua Cranmer Thunderbird and DXR developer Source code archæologist ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Improving visibility of compiler warnings
On Thu, May 25, 2017 at 7:43 AM, Eric Rescorla wrote: > I'd like to second Ehsan's point, but also expand upon it into a more > general observation. > > As it becomes progressively more difficult to build Firefox without mach, > it becomes > increasingly important that mach respect people's workflows. For those of > us who > were comfortable with make and the behavior of having relatively > unfiltered access > to what the build system is doing, it would be great if mach could have > some set of > flags to preserve that view (cf. the flags to remove the timestamps so > that emacs > compiler mode works). > `mach help build` says the answer to this particular problem is `mach --log-no-times build`. And if you really don't like the time prefixing, set the MACH_NO_WRITE_TIMES environment variable. FWIW the times are there to give developers a sense of progress. Teaching the machines to estimate progress/ETA for us is a lot more work than feeding a low-noise signal into the superb pattern recognition apparatus that is the human brain. > > On Thu, May 25, 2017 at 8:31 PM, Ehsan Akhgari > wrote: > >> On 05/19/2017 02:44 PM, Gregory Szorc wrote: >> >>> `mach build` attempts to parse compiler warnings to a persisted >>> "database." >>> You can view a list of compiler warnings post build by running `mach >>> warnings-list`. The intent behind this feature was to make it easier to >>> find and fix compiler warnings. After all, something out of sight is out >>> of >>> mind. >>> >>> There have been a few recent changes to increase the visibility of >>> compiler >>> warnings with the expectation being that raising visibility will increase >>> the chances of someone addressing them. After all, a compiler warning is >>> either a) valid and should be fixed or b) invalid and should be ignored. >>> Either way, a compiler warning shouldn't exist. >>> >> >> Since mystor and billm's objection seems to have gone unaddressed in this >> thread, let me try once again. There is a (c) scenario that you are >> ignoring here: >> >> c) the warning is valid and should be fixed but it's not more important >> than other things than the developer may want to do at the moment. I would >> like to posit that this is often the case. Let's look at what I get these >> days at the end of a normal debug build on Linux. This output is long and >> I am intentionally pasting the full thing here in order to make a point on >> how much useless information we are presenting at the end of each build: >> >> 18:39.34 warning: db/sqlite3/src/sqlite3.c:131236:39 >> [-Wunreachable-code] code will never be executed >> 18:39.34 warning: db/sqlite3/src/sqlite3.c:131292:39 >> [-Wunreachable-code] code will never be executed >> 18:39.34 warning: db/sqlite3/src/sqlite3.c:137670:9 [-Wunreachable-code] >> code will never be executed >> 18:39.34 warning: gfx/angle/src/compiler/translator/ASTMetadataHLSL.cpp:93:15 >> [-Wimplicit-fallthrough] unannotated fall-through between switch labels >> 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:1123:9 >> [-Wimplicit-fallthrough] unannotated fall-through between switch labels >> 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:3640:9 >> [-Wimplicit-fallthrough] unannotated fall-through between switch labels >> 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:3808:9 >> [-Wimplicit-fallthrough] unannotated fall-through between switch labels >> 18:39.34 warning: gfx/cairo/libpixman/src/pixman-bits-image.c:268:32 >> [-Wshift-negative-value] shifting a negative signed value is undefined >> 18:39.34 warning: gfx/cairo/libpixman/src/pixman-linear-gradient.c:395:6 >> [-Wunreachable-code] code will never be executed >> 18:39.34 warning: gfx/cairo/libpixman/src/pixman-x86.c:80:5 >> [-Wexpansion-to-defined] macro expansion producing 'defined' has undefined >> behavior >> 18:39.34 warning: gfx/cairo/libpixman/src/pixman-x86.c:119:5 >> [-Wexpansion-to-defined] macro expansion producing 'defined' has undefined >> behavior >> 18:39.34 warning: intl/hyphenation/hyphen/hyphen.c:332:27 >> [-Wsign-compare] comparison of integers of different signs: 'int' and >> 'unsigned long' >> 18:39.34 warning: intl/icu/source/common/locdspnm.cpp:286:14 >> [-Wunused-private-field] private field 'capitalizationBrkIter' is not used >> 18:39.34 warning: intl/icu/source/common/udataswp.c:438:29 >> [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka >> 'int') and 'unsigned long' >> 18:39.34 warning: intl/icu/source/common/ulist.c:161:24 [-Wsign-compare] >> comparison of integers of different signs: 'int32_t' (aka 'int') and >> 'unsigned long' >> 18:39.34 warning: intl/icu/source/common/uloc_tag.c:1374:31 >> [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka >> 'int') and 'unsigned long' >> 18:39.34 warning: intl/icu/source/common/uloc_tag.c:1409:36 >> [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka >> 'int') and 'unsigned l
Re: Improving visibility of compiler warnings
On Thu, May 25, 2017 at 5:31 AM, Ehsan Akhgari wrote: > On 05/19/2017 02:44 PM, Gregory Szorc wrote: > >> `mach build` attempts to parse compiler warnings to a persisted >> "database." >> You can view a list of compiler warnings post build by running `mach >> warnings-list`. The intent behind this feature was to make it easier to >> find and fix compiler warnings. After all, something out of sight is out >> of >> mind. >> >> There have been a few recent changes to increase the visibility of >> compiler >> warnings with the expectation being that raising visibility will increase >> the chances of someone addressing them. After all, a compiler warning is >> either a) valid and should be fixed or b) invalid and should be ignored. >> Either way, a compiler warning shouldn't exist. >> > > Since mystor and billm's objection seems to have gone unaddressed in this > thread, let me try once again. There is a (c) scenario that you are > ignoring here: > > c) the warning is valid and should be fixed but it's not more important > than other things than the developer may want to do at the moment. I would > like to posit that this is often the case. Let's look at what I get these > days at the end of a normal debug build on Linux. This output is long and > I am intentionally pasting the full thing here in order to make a point on > how much useless information we are presenting at the end of each build: > > 18:39.34 warning: db/sqlite3/src/sqlite3.c:131236:39 [-Wunreachable-code] > code will never be executed > 18:39.34 warning: db/sqlite3/src/sqlite3.c:131292:39 [-Wunreachable-code] > code will never be executed > 18:39.34 warning: db/sqlite3/src/sqlite3.c:137670:9 [-Wunreachable-code] > code will never be executed > 18:39.34 warning: gfx/angle/src/compiler/translator/ASTMetadataHLSL.cpp:93:15 > [-Wimplicit-fallthrough] unannotated fall-through between switch labels > 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:1123:9 > [-Wimplicit-fallthrough] unannotated fall-through between switch labels > 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:3640:9 > [-Wimplicit-fallthrough] unannotated fall-through between switch labels > 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:3808:9 > [-Wimplicit-fallthrough] unannotated fall-through between switch labels > 18:39.34 warning: gfx/cairo/libpixman/src/pixman-bits-image.c:268:32 > [-Wshift-negative-value] shifting a negative signed value is undefined > 18:39.34 warning: gfx/cairo/libpixman/src/pixman-linear-gradient.c:395:6 > [-Wunreachable-code] code will never be executed > 18:39.34 warning: gfx/cairo/libpixman/src/pixman-x86.c:80:5 > [-Wexpansion-to-defined] macro expansion producing 'defined' has undefined > behavior > 18:39.34 warning: gfx/cairo/libpixman/src/pixman-x86.c:119:5 > [-Wexpansion-to-defined] macro expansion producing 'defined' has undefined > behavior > 18:39.34 warning: intl/hyphenation/hyphen/hyphen.c:332:27 > [-Wsign-compare] comparison of integers of different signs: 'int' and > 'unsigned long' > 18:39.34 warning: intl/icu/source/common/locdspnm.cpp:286:14 > [-Wunused-private-field] private field 'capitalizationBrkIter' is not used > 18:39.34 warning: intl/icu/source/common/udataswp.c:438:29 > [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka > 'int') and 'unsigned long' > 18:39.34 warning: intl/icu/source/common/ulist.c:161:24 [-Wsign-compare] > comparison of integers of different signs: 'int32_t' (aka 'int') and > 'unsigned long' > 18:39.34 warning: intl/icu/source/common/uloc_tag.c:1374:31 > [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka > 'int') and 'unsigned long' > 18:39.34 warning: intl/icu/source/common/uloc_tag.c:1409:36 > [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka > 'int') and 'unsigned long' > 18:39.34 warning: intl/icu/source/common/ures_cnv.c:46:18 > [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka > 'int') and 'unsigned long' > 18:39.34 warning: intl/icu/source/common/ures_cnv.c:64:22 > [-Wsign-compare] comparison of integers of different signs: 'int32_t' (aka > 'int') and 'unsigned long' > 18:39.34 warning: intl/icu/source/common/ushape.cpp:1247:12 [-Wcomma] > possible misuse of comma operator here > 18:39.34 warning: intl/icu/source/common/ustring.cpp:860:57 [-Wcomma] > possible misuse of comma operator here > 18:39.34 warning: intl/icu/source/common/ustring.cpp:870:57 [-Wcomma] > possible misuse of comma operator here > 18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:488:47 [-Wcomma] > possible misuse of comma operator here > 18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:535:47 [-Wcomma] > possible misuse of comma operator here > 18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:609:51 [-Wcomma] > possible misuse of comma operator here > 18:39.34 warning: intl/icu/source/common/ustrtrns.cpp:655:47 [-Wcomma] > possible misuse of comma operator h
Re: Improving visibility of compiler warnings
On Fri, May 26, 2017 at 1:50 AM, Gregory Szorc wrote: > On Thu, May 25, 2017 at 7:43 AM, Eric Rescorla wrote: > >> I'd like to second Ehsan's point, but also expand upon it into a more >> general observation. >> >> As it becomes progressively more difficult to build Firefox without mach, >> it becomes >> increasingly important that mach respect people's workflows. For those of >> us who >> were comfortable with make and the behavior of having relatively >> unfiltered access >> to what the build system is doing, it would be great if mach could have >> some set of >> flags to preserve that view (cf. the flags to remove the timestamps so >> that emacs >> compiler mode works). >> > > `mach help build` says the answer to this particular problem is `mach > --log-no-times build`. And if you really don't like the time prefixing, set > the MACH_NO_WRITE_TIMES environment variable. > Yes, I'm aware of this, as I was one of the people who requested it. The point I was making is that it was an example of something that, like this, was a change to the mach output format that some people may have appreciated but also disrupted people's existing work flows. FWIW the times are there to give developers a sense of progress. Teaching > the machines to estimate progress/ETA for us is a lot more work than > feeding a low-noise signal into the superb pattern recognition apparatus > that is the human brain. > I'm not saying that some people don't like it. I'm saying that others don't and because one more or less now needs to use mach, changes which are improvements for some and regressions for others need to be made with extreme care. -Ekr > >> >> On Thu, May 25, 2017 at 8:31 PM, Ehsan Akhgari >> wrote: >> >>> On 05/19/2017 02:44 PM, Gregory Szorc wrote: >>> `mach build` attempts to parse compiler warnings to a persisted "database." You can view a list of compiler warnings post build by running `mach warnings-list`. The intent behind this feature was to make it easier to find and fix compiler warnings. After all, something out of sight is out of mind. There have been a few recent changes to increase the visibility of compiler warnings with the expectation being that raising visibility will increase the chances of someone addressing them. After all, a compiler warning is either a) valid and should be fixed or b) invalid and should be ignored. Either way, a compiler warning shouldn't exist. >>> >>> Since mystor and billm's objection seems to have gone unaddressed in >>> this thread, let me try once again. There is a (c) scenario that you are >>> ignoring here: >>> >>> c) the warning is valid and should be fixed but it's not more important >>> than other things than the developer may want to do at the moment. I would >>> like to posit that this is often the case. Let's look at what I get these >>> days at the end of a normal debug build on Linux. This output is long and >>> I am intentionally pasting the full thing here in order to make a point on >>> how much useless information we are presenting at the end of each build: >>> >>> 18:39.34 warning: db/sqlite3/src/sqlite3.c:131236:39 >>> [-Wunreachable-code] code will never be executed >>> 18:39.34 warning: db/sqlite3/src/sqlite3.c:131292:39 >>> [-Wunreachable-code] code will never be executed >>> 18:39.34 warning: db/sqlite3/src/sqlite3.c:137670:9 >>> [-Wunreachable-code] code will never be executed >>> 18:39.34 warning: >>> gfx/angle/src/compiler/translator/ASTMetadataHLSL.cpp:93:15 >>> [-Wimplicit-fallthrough] unannotated fall-through between switch labels >>> 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:1123:9 >>> [-Wimplicit-fallthrough] unannotated fall-through between switch labels >>> 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:3640:9 >>> [-Wimplicit-fallthrough] unannotated fall-through between switch labels >>> 18:39.34 warning: gfx/angle/src/compiler/translator/ParseContext.cpp:3808:9 >>> [-Wimplicit-fallthrough] unannotated fall-through between switch labels >>> 18:39.34 warning: gfx/cairo/libpixman/src/pixman-bits-image.c:268:32 >>> [-Wshift-negative-value] shifting a negative signed value is undefined >>> 18:39.34 warning: gfx/cairo/libpixman/src/pixman-linear-gradient.c:395:6 >>> [-Wunreachable-code] code will never be executed >>> 18:39.34 warning: gfx/cairo/libpixman/src/pixman-x86.c:80:5 >>> [-Wexpansion-to-defined] macro expansion producing 'defined' has undefined >>> behavior >>> 18:39.34 warning: gfx/cairo/libpixman/src/pixman-x86.c:119:5 >>> [-Wexpansion-to-defined] macro expansion producing 'defined' has undefined >>> behavior >>> 18:39.34 warning: intl/hyphenation/hyphen/hyphen.c:332:27 >>> [-Wsign-compare] comparison of integers of different signs: 'int' and >>> 'unsigned long' >>> 18:39.34 warning: intl/icu/source/common/locdspnm.cpp:286:14 >>> [-Wunused-private-field] private field 'capitalizationBrkIter' is not