Closed Bug 1583717 (Wempty-init-stmt) Opened 1 year ago Closed 11 months ago

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

Categories

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

task

Tracking

(firefox-esr68 wontfix, firefox70 wontfix, firefox71 wontfix, firefox72 wontfix, firefox73 fixed)

RESOLVED FIXED
mozilla73
Tracking Status
firefox-esr68 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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')

(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?

(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

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...

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.

Assignee: nobody → cpeterson
Alias: Wempty-init-stmt
Blocks: buildwarning

-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())
  ;
}
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db127fef7774
Enable clang's -Wempty-init-stmt warnings. r=glandium
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
You need to log in before you can comment on or make changes to this bug.