1583717 - (Wempty-init-stmt) Enable -Wempty-init-stmt warnings after we compile with -std=c++17

Status: RESOLVED FIXED

(Firefox Build System :: General, task, P3)

Description

[Chris Peterson [:cpeterson]]

After we compile with -std=c++17, we should enable clang 8.0's new -Wempty-init-stmt flag for empty initializers in if and switch statements in C++17 code.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0305r1.html

http://releases.llvm.org/8.0.0/tools/clang/docs/ReleaseNotes.html#major-new-features

-Wempty-init-stmt is a new diagnostic that diagnoses empty init-statements of if, switch, range-based for, unless: the semicolon directly follows a macro that was expanded to nothing or if the semicolon is within the macro itself (both macros from system headers, and normal macros).

void test() {
  if(; // <- warning: init-statement of 'if' is a null statement
     true)
    ;

  switch (; // <- warning: init-statement of 'switch' is a null statement
          x) {
    ...
  }

  for (; // <- warning: init-statement of 'range-based for' is a null statement
       int y : S())
    ;
}

https://clang.llvm.org/docs/DiagnosticsReference.html#wempty-init-stmt

As mozilla-central currently has no C++17 code, we have zero -Wempty-init-stmt warnings. :) However, enabling -Wempty-init-stmt implicitly enables clang 8.0's new -Wextra-semi-stmt flag and we have 4626 of those warnings. :( So this patch to enable -Wempty-init-stmt will also need to suppress -Wextra-semi-stmt warnings like:

# catches empty if/switch/for initialization statements that have no effect
add_gcc_warning('-Wempty-init-stmt', cxx_compiler) # C++17 warning
add_gcc_warning('-Wno-extra-semi-stmt')

Comment 1

[Nathan Froyd [:froydnj]]

(In reply to Chris Peterson [:cpeterson] from comment #0)

As mozilla-central currently has no C++17 code, we have zero -Wempty-init-stmt warnings. :) However, enabling -Wempty-init-stmt implicitly enables clang 8.0's new -Wextra-semi-stmt flag and we have 4626 of those warnings. :(

-Wextra-semi-stmt seems pretty pedantic. :( But possibly not that hard to get rid of?

Comment 2

[Chris Peterson [:cpeterson]]

(In reply to Nathan Froyd [:froydnj] from comment #1)

-Wextra-semi-stmt seems pretty pedantic. :( But possibly not that hard to get rid of?

Yes. Most of these warnings are about macros with unnecessary trailing semicolons. They would be simple to fix, but would touch a lot of files and none of them seem to be actual bugs that would cause problems.

js/src/wasm/WasmBaselineCompile.cpp:10923:36 [-Wextra-semi-stmt] empty expression statement has no effect; remove unnecessary ';' to silence this warning

https://searchfox.org/mozilla-central/rev/f43ae7e1c43a4a940b658381157a6ea6c5a185c1/js/src/wasm/WasmBaselineCompile.cpp#10923

Comment 3

[Sylvestre Ledru [:Sylvestre]]

js/src/wasm/WasmBaselineCompile.cpp:10923:36 [-Wextra-semi-stmt] empty expression statement has no effect; remove unnecessary ';' to silence this warning

https://searchfox.org/mozilla-central/rev/f43ae7e1c43a4a940b658381157a6ea6c5a185c1/js/src/wasm/WasmBaselineCompile.cpp#10923

We could argue that in that case, it is a bug in clang...

Comment 4

[Mike Hommey [:glandium]]

Considering the definition of CHECK_NEXT (https://searchfox.org/mozilla-central/source/js/src/wasm/WasmBaselineCompile.cpp#10858-10863), clang is right.

Comment 5

[Chris Peterson [:cpeterson]]

Green Try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e1abb3333a497e36623334921e155ca46808d7b

I didn't need to suppress -Wextra-semi-stmt warnings this time (like in comment 0). I confirmed that clang changed -Wempty-init-stmt to no longer imply -Wextra-semi-stmt. mozilla-central used to have 4000+ -Wextra-semi-stmt warnings and not really worth fixing.

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: Bug 1583717 - Enable clang's -Wempty-init-stmt warnings. r?glandium

-Wempty-init-stmt is a new clang diagnostic that diagnoses empty C++17 init-statements of if, switch, and range-based for statements:

void test() {
  if(; // <- warning: init-statement of 'if' is a null statement
    true)
  ;

  switch (; // <- warning: init-statement of 'switch' is a null statement
    x) {
    ...
  }

  for (; // <- warning: init-statement of 'range-based for' is a null statement
    int y : S())
  ;
}

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db127fef7774
Enable clang's -Wempty-init-stmt warnings. r=glandium

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/db127fef7774