Closed Bug 1577319 Opened 7 years ago Closed 6 years ago

stop using `using namespace std` at global scope

Categories

(Core :: General, defect, P2)

Product:
Core
Component:
General
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: froydnj, Unassigned)

References

Details

INFO -  z:/build/fetches/sccache/sccache.exe z:/build/fetches/clang/bin/clang-cl.exe -Xclang -std=c++17 --target=aarch64-windows-msvc -FoUnified_cpp_ipc_glue1.obj -c -flto=thin -fuse-ld=lld -Iz:/build/build/src/obj-firefox/dist/stl_wrappers -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_WIN -DHAVE_WINSOCK2_H -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DCOMPILER_MSVC '-DMOZ_CHILD_PROCESS_NAME="plugin-container.exe"' '-DMOZ_CHILD_PROCESS_BUNDLE="plugin-container.app/Contents/MacOS/"' -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Iz:/build/build/src/ipc/glue -Iz:/build/build/src/obj-firefox/ipc/glue -Iz:/build/build/src/caps -Iz:/build/build/src/dom/broadcastchannel -Iz:/build/build/src/dom/indexedDB -Iz:/build/build/src/dom/storage -Iz:/build/build/src/media/webrtc/trunk -Iz:/build/build/src/media/webrtc/trunk/webrtc -Iz:/build/build/src/xpcom/build -Iz:/build/build/src/dom/ipc -Iz:/build/build/src/toolkit/crashreporter -Iz:/build/build/src/toolkit/xre -Iz:/build/build/src/xpcom/base -Iz:/build/build/src/xpcom/threads -Iz:/build/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -Iz:/build/build/src/ipc/chromium/src -Iz:/build/build/src/ipc/glue -Iz:/build/build/src/security/sandbox/chromium -Iz:/build/build/src/security/sandbox/chromium-shim -Iz:/build/build/src/security/sandbox/win/src/sandboxbroker -Iz:/build/build/src/obj-firefox/dist/include -Iz:/build/build/src/obj-firefox/dist/include/nspr -Iz:/build/build/src/obj-firefox/dist/include/nss -MD -FI z:/build/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -Qunused-arguments -guard:cf -Qunused-arguments -fcrash-diagnostics-dir=z:/build/public/build -TP -nologo -Zc:sizedDealloc- -D_HAS_EXCEPTIONS=0 -guard:cf -W3 -Gy -Zc:inline -Gw -Wno-inline-new-delete -Wno-invalid-offsetof -Wno-microsoft-enum-value -Wno-microsoft-include -Wno-unknown-pragmas -Wno-ignored-pragmas -Wno-deprecated-declarations -Wno-invalid-noreturn -Wno-inconsistent-missing-override -Wno-implicit-exception-spec-mismatch -Wno-unused-local-typedef -Wno-ignored-attributes -Wno-used-but-marked-unused -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -O2 -Oy- -WX  -Xclang -MP -Xclang -dependency-file -Xclang .deps/Unified_cpp_ipc_glue1.obj.pp -Xclang -MT -Xclang Unified_cpp_ipc_glue1.obj   z:/build/build/src/obj-firefox/ipc/glue/Unified_cpp_ipc_glue1.cpp
INFO -  In file included from z:/build/build/src/obj-firefox/ipc/glue/Unified_cpp_ipc_glue1.cpp:65:
INFO -  In file included from z:/build/build/src/ipc/glue/ProtocolUtils.cpp:32:
INFO -  In file included from z:/build/build/src/vs2017_15.9.6/SDK\include\10.0.17134.0\um\aclapi.h:23:
INFO -  In file included from z:/build/build/src/vs2017_15.9.6/SDK\include\10.0.17134.0\um\accctrl.h:17:
INFO -  In file included from z:/build/build/src/vs2017_15.9.6/SDK\include\10.0.17134.0\shared\wtypes.h:22:
INFO -  z:/build/build/src/vs2017_15.9.6/SDK\include\10.0.17134.0\shared/rpcndr.h(192,9): error: reference to 'byte' is ambiguous
INFO -  typedef byte cs_byte;
INFO -          ^
INFO -  z:/build/build/src/vs2017_15.9.6/SDK\include\10.0.17134.0\shared/rpcndr.h(191,23): note: candidate found by name lookup is 'byte'
INFO -  typedef unsigned char byte;
INFO -                        ^
INFO -  z:\build\build\src\vs2017_15.9.6\VC\include\cstddef(23,12): note: candidate found by name lookup is 'std::byte'
INFO -  enum class byte : unsigned char { };
INFO -             ^
INFO -  In file included from z:/build/build/src/obj-firefox/ipc/glue/Unified_cpp_ipc_glue1.cpp:65:
INFO -  In file included from z:/build/build/src/ipc/glue/ProtocolUtils.cpp:32:
INFO -  In file included from z:/build/build/src/vs2017_15.9.6/SDK\include\10.0.17134.0\um\aclapi.h:23:
INFO -  In file included from z:/build/build/src/vs2017_15.9.6/SDK\include\10.0.17134.0\um\accctrl.h:17:
INFO -  In file included from z:/build/build/src/vs2017_15.9.6/SDK\include\10.0.17134.0\shared\wtypes.h:22:
INFO -  z:/build/build/src/vs2017_15.9.6/SDK\include\10.0.17134.0\shared/rpcndr.h(962,6): error: reference to 'byte' is ambiguous
INFO -       byte               *   pNetworkData,
INFO -       ^
INFO -  z:/build/build/src/vs2017_15.9.6/SDK\include\10.0.17134.0\shared/rpcndr.h(191,23): note: candidate found by name lookup is 'byte'
INFO -  typedef unsigned char byte;
INFO -                        ^
INFO -  z:\build\build\src\vs2017_15.9.6\VC\include\cstddef(23,12): note: candidate found by name lookup is 'std::byte'
INFO -  enum class byte : unsigned char { };
INFO -             ^

...and so on in the same vein for quite some time.

Maybe we could try just...not including those files? I'm not really sure what aclapi.h contributes here (DuplicateHandle?). I'm kind of at a loss as how to fix this.

Maybe a newer SDK has some kind of header file workaround?

Blocks: C++17

That's an odd one. I have no idea why we would need to include aclapi.h there.

I suspect that aclapi.h was added for the ::GetSecurityInfo in https://hg.mozilla.org/mozilla-central/rev/4091419d6b1c6a254da04190545e45a0c3f2833a -- which no longer seems to be around?

Alternatively I wonder if we can fix this by de-unifying ProtocolUtils.cpp so that it doesn't come after someone who did using namespace std earlier.

Or just individually using std::*, or just explicitly std::-qualify the relevant symbols. Not like opening up namespaces that way is particularly good form...

using namespace is not actually allowed by the style guide: https://google.github.io/styleguide/cppguide.html#Namespaces

(In reply to Simon Giesecke [:sg] [he/him] from comment #5)

using namespace is not actually allowed by the style guide: https://google.github.io/styleguide/cppguide.html#Namespaces

😂 This is a place where our ostensible adherence to the terrible Google style guide is utter lies. (Tho to be fair in the newsgroups we'll admit we do this weird amalgam of the guide plus some deviations.) It cannot be exaggerated how far we diverge from that:

https://searchfox.org/mozilla-central/search?q=using+namespace+.%2B%3B&case=false&regexp=true&path=

(In reply to Jeff Walden [:Waldo] from comment #6)

(In reply to Simon Giesecke [:sg] [he/him] from comment #5)

using namespace is not actually allowed by the style guide: https://google.github.io/styleguide/cppguide.html#Namespaces

😂 This is a place where our ostensible adherence to the terrible Google style guide is utter lies. (Tho to be fair in the newsgroups we'll admit we do this weird amalgam of the guide plus some deviations.) It cannot be exaggerated how far we diverge from that:

https://searchfox.org/mozilla-central/search?q=using+namespace+.%2B%3B&case=false&regexp=true&path=

Yes, that's true, and I am aware of that. I am going through the Google style guide right now, and am checking where we actually conform, and where don't conform but conformance would be good. There are also some points which we cannot or would not want to conform to.

However, I think you already pointed out this is a point where it would be good if we moved to conformance :)

Thing is, I somewhat doubt this can be the thing to push us over that line. I would like to be proven wrong, but I don't expect I would be.

I expect if we pushed to remove using namespace * everywhere we would encounter very, very massive resistance. Maybe worth pushing through at some point, but this is probably hackaroundable and probably can't be the thing to sell the change.

Apparently the header is unused, but we do run into these same sort of issues elsewhere in the tree:

 INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/gfx/layers/Unified_cpp_gfx_layers11.cpp:38:
 INFO -  In file included from /builds/worker/workspace/build/src/gfx/layers/mlgpu/TextureSourceProviderMLGPU.cpp:11:
 INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/MLGDeviceD3D11.h:10:
 INFO -  In file included from /builds/worker/fetches/clang/bin/../i686-w64-mingw32/include/d3d11_1.h:8:
ERROR -  /builds/worker/fetches/clang/bin/../i686-w64-mingw32/include/rpcndr.h:64:11: error: reference to 'byte' is ambiguous
 INFO -    typedef byte cs_byte;
 INFO -            ^
 INFO -  /builds/worker/fetches/clang/bin/../i686-w64-mingw32/include/rpcndr.h:63:25: note: candidate found by name lookup is 'byte'
 INFO -    typedef unsigned char byte;
 INFO -                          ^
 INFO -  /builds/worker/fetches/clang/bin/../i686-w64-mingw32/include/c++/v1/cstddef:66:12: note: candidate found by name lookup is 'std::byte'
 INFO -  enum class byte : unsigned char {};
 INFO -             ^

and that sort of case is not so easily handled by deleting header includes.

I also don't really want to un-unify a bunch of files just so we can work around windows headers. I think we may have to go the comment 7 route and at least attempt to eliminate using namespace std?

I also don't really want to un-unify a bunch of files just so we can work around windows headers.

In fairness to those headers, I think this is more of a "thing we brought on ourselves with using namespace std and unified compilation" than something that the headers did. :-)

Un-using namespace std (at least to the extent of fixing the build) does seem like a good option. It's a change that would be virtuous even in the absence of this specific bug.

In light of this std::byte issue causing problems with C++17 and also being in conflict with the style guide, I am going to re-purpose this bug to eliminate using namespace std; at global scope. From a brief trip through the instances that we have in-tree, it looks like people also do using namespace std; at function-level scope, which is still not conformant with the style guide, but probably causes fewer issues. If we happen to fix such instances along the way, though, I don't think anybody is going to complain.

The core loop of eliminating this pattern looks something like:

  1. Find a using namespace std; declaration at global scope in Mozilla code. We appear to have a number of them in third-party code, which ideally will not cause any issues.
  2. Delete it.
  3. Compile.
  4. Fix compile errors resulting from said deletion, if any. Note that you may encounter errors outside of the file due to unified compilation.
  5. Goto 1.
  6. If there are still errors in third-party code, we have a couple of options:
    6a. Put the relevant files in SOURCES in moz.build files instead of UNIFIED_SOURCES.
    6b. Fix things upstream and import new sources.
    6c. Set -std=gnu++14//std=c++14 on the files as appropriate.
  7. Choose the appropriate strategy for the third-party code in question.
  8. Goto 6.
  9. Write a linter to make sure that this never happens again in Mozilla code.
Component: IPC → General
OS: Windows → All
Summary: name conflicts between cstddef and rpcndr.h on Windows with -std=c++17, triggered on ipc/glue/ProtocolUtils.cpp → stop using `using namespace std` at global scope
Depends on: 1577831
Depends on: 1577867
Depends on: 1577869
Depends on: 1577871
Depends on: 1577885
Depends on: 1577910
Depends on: 1577916

I have patches up for:

gfx/2d/
gfx/layers/
image/
js/
media/gmp-clearkey/
toolkit/

Nathan has one for ipc/, and Gerald for mozglue/ and tools/. I believe that leaves the following non-thirdparty code:

browser/components/
dom/
gfx/src/
gfx/vr/

And the following thirdparty code:

intl/icu/
media/libcubeb/
media/mtransport/third_party/nICEr/
media/webrtc/
other-licenses/nsis/
other-licenses/snappy/
security/nss/
third_party/rlbox/
third_party/rust/cubeb-sys/ (this is a repeat of libcubeb I think)

I think we can pretty easily update cubeb, nss, and rlbox. I'm not sure what the story is on the rest.

Priority: -- → P2
Depends on: 1577932
Depends on: 1577934
Depends on: 1577936

dom/ is covered as well, that leaves:

browser/components/
gfx/src/
gfx/vr/

AFAICT the browser/components/translation/cld2 stuff is just emscripten'd, not built into our binary so we can probably ignore that. Does that sound right Florian?

Flags: needinfo?(florian)
Depends on: 1578024
Depends on: 1578025
Depends on: 1578107

(In reply to Eric Rahm [:erahm] from comment #14)

AFAICT the browser/components/translation/cld2 stuff is just emscripten'd, not built into our binary so we can probably ignore that. Does that sound right Florian?

Yes.

Flags: needinfo?(florian)

I think we can call this done enough. We might want to file a follow up to add linter to avoid new additions.

Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.