1674448 - Consider not using -Werror for base toolchain builds?

Status: RESOLVED INVALID

(Firefox Build System :: General, task)

Description

[Emilio Cobos Álvarez [:emilio]]

https://treeherder.mozilla.org/logviewer?job_id=320246304&repo=autoland&lineNumber=79556

is a GCC-only warning (and also I think a false-positive). The patch added a static function with the same name (but different arguments) than a virtual function.

I had to send a quick fix to keep the tree green: https://hg.mozilla.org/integration/autoland/rev/4c99eba6641e71524bcb00686e34e8d852e1f1d4

But it's a bit unfortunate to keep these -Werror warnings when we intentionally use "old" compilers in these builds.

Comment 1

[Simon Giesecke [:sg] [he/him]]

I hit another instance of this, I got a backout for a base-toolchain bustage, with the baseline clang version in this case, see https://bugzilla.mozilla.org/show_bug.cgi?id=1583109#c7

It fails because of a -Wrange-loop-analysis warning in a template, which isn't emitted by more recent clang versions. I think I need to manually disable the warning in that file to get around it. See also https://quuxplusone.github.io/blog/2020/08/26/wrange-loop-analysis/ which argues against this particular warning that is not included in either -Wall or -Wextra, and also abseil's rationale for keeping it disabled (https://abseil.io/docs/cpp/platforms/compilerflags#clang-flags), the reason also applying to the particular case we have here, since the value type is an enum type here.

Maybe this is a slightly different case, as it might be appropriate to just disable this particular warning, even if deciding against removing -Werror completely. (Although like Emilio I think that would be a good idea as well)

Mitchell, WDYT, should I file a separate bug?

Comment 2

[Simon Giesecke [:sg] [he/him]]

In any case, the pragmas in StringJoinAppend in xpcom/string/nsReadableUtils.h added by https://phabricator.services.mozilla.com/D98750 should be removed after this.

Comment 3

[Mitchell Hentges [:mhentges] 🦀]

Hey Simon, I think that that should be a separate ticket. I believe that the solution to Emilio's report will be something specific to the problem mentioned (rather than removing -Werror). I'll create yours shortly here.

---

is a GCC-only warning (and also I think a false-positive).

But it's a bit unfortunate to keep these -Werror warnings when we intentionally use "old" compilers in these builds.

FWIW, this warning is isn't just on "old" GCC, it's reproducible on GCC 11, too.
Additionally, I think that gcc has the correct behaviour here: if a parent's virtual function is overloaded, then either:

• You explicitly make it still available:

      public:
          using gfxFontEntry::HasFontTable;
  

• You leave it as-is and implicitly make a virtual function un-callable, which the warning is trying to avoid:

      void some_func(gfxFT2FontBase* gfxFont) {
          fontBase->HasFontTable(1); // Fails, even though the parent class has this method?
      }
  

• You re-name your new functions so they don't overload, as you did.

Clang has the same warning for the exact same reason, but it doesn't warn for static overloads of virtual functions - godbolt for proof.

TL;DR: I think the warning was correct, and it's a bummer that clang didn't catch this, so you weren't able to see the warning in local development.

I'm going to close this here based on what I've understood and described above^. I'm getting up to speed with C++, so if I misunderstood or am missing something, please re-open and comment :)