1010235 - Do not call "using namespace" in global scope for UNIFIED_SOURCES files

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Frederic Plourde (:fred23)]

When building UNIFIED_SOURCE files, one must take great care of *not* calling 'using namespace <xyz>' in global scope (that is, outside of any 'namespace <XYZ> {}' because it might impact other files in the UNIFIED_SOURCES list by causing 'ambiguous symbols'.

Best practice with regards to using namespaces should be :

 1) When 'using' a namespace, always call it from *inside* some namespace scope:
    ex : namespace <xx> { [using namespace <yy>] }. This way, it will not spill over following files in the UNIFIED_SOURCES list.

 2) When 'using' a namespace from a source file *with no* namespace scope declaration, simply REMOVE this file from the UNIFIED_SOURCES list (in the corresponding moz.build file) and put it in the SOURCES list.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Difficult problem and constraints. We don't want to force bigger departures from usual practice than necessary, and at the same time we want to get the highest possible benefit from UNIFIED_SOURCES in terms of compilation speed.

I wouldn't necessarily go as far as what is suggested in the above Description, but I've found that the following approach worked well enough as far as I could see:

 1) In new code, prefer doing "namespace foo { ... }" instead of "using namespace foo;" at global scope, even in .cpp files.  (This is of course standard practice in headers; what is new is to apply that to non-header files too).

 2) When running into a build issue with UNIFIED_SOURCES, consider applying 1) to existing code causing such issues.

Comment 2

[Frederic Plourde (:fred23)]

Right,

and in fact, I have created this bug because of actual troubles caused by files (above mine in the gfx/layers/moz.build UNIFIED_SOURCES list) not following those best practices.

I will have patches posted herein shortly, fixing my specific problems, and will need those to go in asap.

... But may I suggest that we keep this bug open for future references so that more people follow those best practices for new files ?

Comment 3

[Frederic Plourde (:fred23)]

(In reply to Benoit Jacob [:bjacob] from comment #1)
> I wouldn't necessarily go as far as what is suggested in the above
> Description, but I've found that the following approach worked well enough
> as far as I could see:
> 
>  1) In new code, prefer doing "namespace foo { ... }" instead of "using
> namespace foo;" at global scope, even in .cpp files.  (This is of course
> standard practice in headers; what is new is to apply that to non-header
> files too).
> 
>  2) When running into a build issue with UNIFIED_SOURCES, consider applying
> 1) to existing code causing such issues.

This is good, although I'm afraid it won't work with some files, as this one here : 
http://lxr.mozilla.org/mozilla-central/source/gfx/layers/Effects.cpp

the "using namespace mozilla::layers;" is causing me troubles, but I can't nest it inside any already present namespace foo {} and creating one, I'm afraid, will impact other code using those PrintInfo methods... The only thing I can think of for this one (and there are others) is to remove it from the UNIFIED_SOURCES list and put it in the SOURCES list.

whant do you think ?
thanks

Comment 4

[Frederic Plourde (:fred23)]

Attachment: Put_using_namespace_in_namespace_scope.patch

If namespace mozilla::gfx in all those files is 'used' from global scope, it makes my work in RotatedBuffer conflict with ambiguous symbols. That's because they are all in the UNIFIED_SOURCES list...

This patch only does what other have already done for some other files, like http://lxr.mozilla.org/mozilla-central/source/gfx/layers/RotatedBuffer.cpp, but does it for *all* the source files in gfx/layers/.

Comment 5

[Frederic Plourde (:fred23)]

Attachment: Get_sources_without_namespace_scope_out_of_unified_sources_list.patch

Here's another patch for source files *without* namespace scopes (like Layers.cpp, which has already been placed in SOURCES long time ago).

benoit : if there's fix for that (instead of moving them OUT of UNIFIED_SOURCES), I'd be glad to do it, thanks :)

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Frederic Plourde (:fred23) from comment #3)
> (In reply to Benoit Jacob [:bjacob] from comment #1)
> > I wouldn't necessarily go as far as what is suggested in the above
> > Description, but I've found that the following approach worked well enough
> > as far as I could see:
> > 
> >  1) In new code, prefer doing "namespace foo { ... }" instead of "using
> > namespace foo;" at global scope, even in .cpp files.  (This is of course
> > standard practice in headers; what is new is to apply that to non-header
> > files too).
> > 
> >  2) When running into a build issue with UNIFIED_SOURCES, consider applying
> > 1) to existing code causing such issues.
> 
> This is good, although I'm afraid it won't work with some files, as this one
> here : 
> http://lxr.mozilla.org/mozilla-central/source/gfx/layers/Effects.cpp
> 
> the "using namespace mozilla::layers;" is causing me troubles, but I can't
> nest it inside any already present namespace foo {} and creating one, I'm
> afraid, will impact other code using those PrintInfo methods... The only
> thing I can think of for this one (and there are others) is to remove it
> from the UNIFIED_SOURCES list and put it in the SOURCES list.
> 
> whant do you think ?
> thanks

Have you tried replacing the "using namespace mozilla::layers" by just "using mozilla::layers::TextureInfo" and whatever other few things are needed here?

If the body of some of these functions really needs many things from namespace mozilla::layers, you can still put using namespace statements inside function bodies.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8422575 [details] [diff] [review]
Get_sources_without_namespace_scope_out_of_unified_sources_list.patch

Review of attachment 8422575 [details] [diff] [review]:
-----------------------------------------------------------------

There _is_ a better fix for that, that does not involve removing files from the unified build.

First consider the trivial case where you have a chain of nested using namespace statements, like:

   using namespace mozilla;
   using namespace mozilla::layers;

You can simply replace that by a chain of nested namespace {...} statements like:

   namespace mozilla {
   namespace layers {

Then consider the less trivial case where you have more using namespace statements that dont fit in such a chain (example: CanvasLayerComposite.cpp), like this:

   using namespace mozilla;
   using namespace mozilla::layers;
   using namespace mozilla::gfx;

You can fix this by choosing what is the most important chain of namespaces there, treat it as above, and address the rest by manual gfx:: prefixes:

   namespace mozilla {
   namespace layers {
   gfx::DoSometing(gfx::Foo);

Finally, I suppose there could be a case of incomplete chains of using namespace statements, like:

   using namespace mozilla::layers;
   using namespace mozilla::gfx;

without the "using namespace mozilla;". I suppose that you could reduce that to the above cases by doing as if there had been a using namespace mozilla.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Note: for all files under gfx/layers, "the most important chain of nested namespaces" is mozilla -> mozilla::layers. Treat namespace gfx as the auxiliary one that can be addressed by keeping manual gfx:: prefixes.

Comment 9

[Frederic Plourde (:fred23)]

Made this block 1015218 because we'll need those patches in before I can land 1015218.

Comment 10

[Frederic Plourde (:fred23)]

Good news, just verified that this was fixed by 

changeset:   184508:0398e7ef814b
user:        Nicolas Silva <[e-mail]>
date:        Thu May 22 12:11:45 2014 +0200
summary:     Bug 1013292 - Fix some using namespace + unified build issues in gfx code. r=kats

So closing this as resolved-fixed.