1443444 - Turn on clang's -Wheader-hygiene warning

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Jonathan Watt [:jwatt]]

Things like `using namespace` in header files are bad.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Example of output:
 In file included from /home/sylvestre/dev/mozilla/mozilla-central.hg/obj-x86_64-pc-linux-gnu/mfbt/tests/gtest/Unified_cpp_mfbt_tests_gtest0.cpp:2:
 /home/sylvestre/dev/mozilla/mozilla-central.hg/mfbt/tests/gtest/TestSpan.cpp:31:17: error: using namespace directive in global context in header [-Werror,-Wheader-hygiene]
 using namespace std;
                 ^
 /home/sylvestre/dev/mozilla/mozilla-central.hg/mfbt/tests/gtest/TestSpan.cpp:32:17: error: using namespace directive in global context in header [-Werror,-Wheader-hygiene]
 using namespace mozilla;
                 ^

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Doesn't work great on unified build. Reported a bug upstream: https://bugs.llvm.org/show_bug.cgi?id=36616

Comment 3

[Sylvestre Ledru [:Sylvestre]]

A lot of issues caused by the unified builds.
Stripping results with cpp in the file name, here is the list.

Most of them (all?) are:
using namespace mozilla;

devtools/shared/heapsnapshot/tests/gtest/DevTools.h:24:17: warning: using namespace directive in global context in header [-Wheader-hygiene]
devtools/shared/heapsnapshot/tests/gtest/DevTools.h:25:26: warning: using namespace directive in global context in header [-Wheader-hygiene]
devtools/shared/heapsnapshot/tests/gtest/DevTools.h:26:26: warning: using namespace directive in global context in header [-Wheader-hygiene]
devtools/shared/heapsnapshot/tests/gtest/DevTools.h:27:17: warning: using namespace directive in global context in header [-Wheader-hygiene]
gfx/layers/apz/test/gtest/APZTestCommon.h:35:17: warning: using namespace directive in global context in header [-Wheader-hygiene]
gfx/layers/apz/test/gtest/APZTestCommon.h:36:26: warning: using namespace directive in global context in header [-Wheader-hygiene]
gfx/layers/apz/test/gtest/APZTestCommon.h:37:26: warning: using namespace directive in global context in header [-Wheader-hygiene]
layout/svg/nsSVGImageFrame.h:29:17: warning: using namespace directive in global context in header [-Wheader-hygiene]
layout/svg/nsSVGImageFrame.h:30:26: warning: using namespace directive in global context in header [-Wheader-hygiene]
layout/svg/nsSVGImageFrame.h:31:26: warning: using namespace directive in global context in header [-Wheader-hygiene]
layout/svg/nsSVGImageFrame.h:32:26: warning: using namespace directive in global context in header [-Wheader-hygiene]
security/certverifier/TrustOverrideUtils.h:14:17: warning: using namespace directive in global context in header [-Wheader-hygiene]
toolkit/components/telemetry/tests/gtest/TelemetryFixture.h:12:17: warning: using namespace directive in global context in header [-Wheader-hygiene]
toolkit/components/url-classifier/nsUrlClassifierDBService.h:66:26: warning: using namespace directive in global context in header [-Wheader-hygiene]
toolkit/components/url-classifier/tests/gtest/Common.h:7:17: warning: using namespace directive in global context in header [-Wheader-hygiene]
toolkit/components/url-classifier/tests/gtest/Common.h:8:26: warning: using namespace directive in global context in header [-Wheader-hygiene]
warning: devtools/shared/heapsnapshot/tests/gtest/DevTools.h:24:17 [-Wheader-hygiene] using namespace directive in global context in header
warning: devtools/shared/heapsnapshot/tests/gtest/DevTools.h:25:26 [-Wheader-hygiene] using namespace directive in global context in header
warning: devtools/shared/heapsnapshot/tests/gtest/DevTools.h:26:26 [-Wheader-hygiene] using namespace directive in global context in header
warning: devtools/shared/heapsnapshot/tests/gtest/DevTools.h:27:17 [-Wheader-hygiene] using namespace directive in global context in header
warning: gfx/layers/apz/test/gtest/APZTestCommon.h:35:17 [-Wheader-hygiene] using namespace directive in global context in header
warning: gfx/layers/apz/test/gtest/APZTestCommon.h:36:26 [-Wheader-hygiene] using namespace directive in global context in header
warning: gfx/layers/apz/test/gtest/APZTestCommon.h:37:26 [-Wheader-hygiene] using namespace directive in global context in header
warning: layout/svg/nsSVGImageFrame.h:29:17 [-Wheader-hygiene] using namespace directive in global context in header
warning: layout/svg/nsSVGImageFrame.h:30:26 [-Wheader-hygiene] using namespace directive in global context in header
warning: layout/svg/nsSVGImageFrame.h:31:26 [-Wheader-hygiene] using namespace directive in global context in header
warning: layout/svg/nsSVGImageFrame.h:32:26 [-Wheader-hygiene] using namespace directive in global context in header
warning: media/webrtc/signaling/gtest/MockCall.h:11:17 [-Wheader-hygiene] using namespace directive in global context in header
warning: security/certverifier/TrustOverrideUtils.h:14:17 [-Wheader-hygiene] using namespace directive in global context in header
warning: toolkit/components/telemetry/tests/gtest/TelemetryFixture.h:12:17 [-Wheader-hygiene] using namespace directive in global context in header
warning: toolkit/components/url-classifier/nsUrlClassifierDBService.h:66:26 [-Wheader-hygiene] using namespace directive in global context in header
warning: toolkit/components/url-classifier/tests/gtest/Common.h:7:17 [-Wheader-hygiene] using namespace directive in global context in header
warning: toolkit/components/url-classifier/tests/gtest/Common.h:8:26 [-Wheader-hygiene] using namespace directive in global context in header
warning: using namespace directive in global context in header [-Wheader-hygiene]


So, bottom line:
* cannot be enabled with unified builds
* the warnings are mostly about our namespace...

We should probably wontfix this.

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #3)
> * the warnings are mostly about our namespace...

That's definitely going to be the case if llvm doesn't distinguish between header files and source files during unified compilation. And from the comments on the upstream bug it sounds like there's no interest in making llvm smarter in this regard. In that case, yes, wontfixing makes sense.

Comment 5

[Jonathan Watt [:jwatt]]

If we really cared about this we could, I guess, write a `mach build' option to go through the tree, compiling each header into a stub source file. That'd find issues with missing includes in headers too.

Not exactly the highest priority given we're doing unified builds though...