812314 - Don't put definitions in `namespace js { ... }` blocks in .cpp files

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

So, apparently it's slightly preferred to do a qualified definition:

    void
    frontend::Foo()
    {
    }

rather than wrap the whole .cpp file in namespace blocks:

    namespace js {
    namespace frontend {

    void Foo()
    {
    }

    } // namespace frontend
    } // namespace js

This was a reversal of what we previously did, though, so we're pretty inconsistent about it. Here's a patch that fixes up a bunch of places to do the former rather than the latter -- not all of them but most of them.

It moves a bunch of static (file-scope) helper functions out of namespaces and into the global namespace, which I'm going to call a feature.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - v1

Oh. In a handful of places, we're using a trick where you sort of declare a function without exposing the declaration it, like this:

    namespace js {

    inline void GoNuts(Squirrel *sq) {
        extern GoNutsSlow(Squirrel *);
        if (!sq->rabid())
            GoNutsSlow(sq);
    }

    }

I get it, but I think we're better off with one less clever thing in the codebase. The patch removes it in favor of actually declaring the Slow version in the header, which was already the more common practice.

Comment 2

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=063ff03e40f6

Comment 3

[Jason Orendorff [:jorendorff]]

Revised:
https://tbpl.mozilla.org/?tree=Try&rev=7a442853a4ca

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - v2

Sorry this is huge. :-\ Very mechanical though.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - gc directory, v2

gc directory. There is some moderately weird stuff going on in Marking.cpp, so a few namespace-blocks remain there.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 682446 [details] [diff] [review]
part 1 - v2

Nicely done sir!  This has bugged me for a long time.  (I think I started the trend the wrong way by wrapping jstracer.cpp in namespace js { ... } before I saw why explicit qualification is better.)  lol @ make_unicode.py

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 682447 [details] [diff] [review]
Part 2 - gc directory, v2

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

\o/  This is very nice.  Thanks for flagging me for review: I learned a bunch.  Questions below are mostly for my own edjikashun, feel free to land after fixing the minor nits I found.

::: js/src/gc/Marking.cpp
@@ +24,1 @@
>  using mozilla::DebugOnly;

Linebreak between these sections, see vm/Stack.cpp for an example.

@@ +118,5 @@
>                   thing->compartment() == rt->atomsCompartment);
>  }
>  
>  template<typename T>
> +static void

Oh, nice catch!

@@ +158,5 @@
>      MarkInternal(trc, thingp);
>  }
>  
> +namespace js {
> +namespace gc {

Why is this required here?  Not saying it's not, I'm just surprised since you didn't need one for MarkRoot and it should have exactly the same sematics and usage.

@@ +238,5 @@
>  {                                                                                                 \
>      MarkUnbarriered<type>(trc, thingp, name);                                                     \
>  }                                                                                                 \
>                                                                                                    \
> +void                                                                                              \

Doh!  I totally missed that.  My brain must have wanted fewer lines to \s*\\.

@@ +936,5 @@
>          PushMarkStack(gcmarker, type->interpretedFunction);
>  }
>  
>  static void
> +gc::MarkChildren(JSTracer *trc, types::TypeObject *type)

This is static.  Why does it (and the following handful) need to be in gc:: whereas the other static functions don't?

::: js/src/gc/Memory.cpp
@@ +13,1 @@
>  #include "mozilla/Assertions.h"

I thought our normal include ordering put mozilla/ at the top?

::: js/src/gc/StoreBuffer.cpp
@@ +18,5 @@
>  /*** MonoTypeBuffer ***/
>  
>  template <typename T>
>  bool
>  StoreBuffer::MonoTypeBuffer<T>::enable(uint8_t *region, size_t len)

Huh, why does leaving off the gc:: work here when it doesn't in the other files?

Comment 8

[Jason Orendorff [:jorendorff]]

evilpies pointed out that I didn't run make_unicode.py and check in the modified Unicode.cpp file. Will fix for checkin.

(In reply to Terrence Cole [:terrence] from comment #7)
> >  using mozilla::DebugOnly;
> Linebreak between these sections, see vm/Stack.cpp for an example.

Thanks.

> > +namespace js {
> > +namespace gc {
> 
> Why is this required here?  Not saying it's not, I'm just surprised since
> you didn't need one for MarkRoot and it should have exactly the same
> sematics and usage.

This is really hard to explain, bear with me...

These namespace blocks contain only the template js::gc::Mark<T>(JSTracer *trc, EncapsulatedPtr<T> *thing, const char *name). You can move this template out of the namespace and it'll parse just fine, but a bit later, the macro generates code like this:

// in namespace js::gc, generated by DeclMarkerImpl(BaseShape, BaseShape)
void
MarkBaseShape(JSTracer *trc, EncapsulatedPtr<BaseShape> *thing, const char *name)
{
    Mark<BaseShape>(trc, thing, name);
}

This fails to compile, because the lookup for 'Mark' finds the functions js::gc::Mark declared in Marking.h. It does not find the global template, because of course a hit in an inner namespace shadows a hit in an outer namespace.

(Extra credit: Furthermore the error message we get will be really baffling, because those none of those js::gc::Mark signatures in Marking.h are templates, so the angle brackets in "Mark<BaseShape>(trc, thing, name)" are parsed as less-than and greater-than operators, and then C++ goes quietly to pieces from there.)

We want the C++ compiler to consider all the Mark signatures at each call site, so we need to put them all in the same namespace.

> @@ +936,5 @@
> >  static void
> > +gc::MarkChildren(JSTracer *trc, types::TypeObject *type)
> 
> This is static.  Why does it (and the following handful) need to be in gc::
> whereas the other static functions don't?

The rule is just that for a top-level definition to match up with a previous declaration in a namespace, the definition has to be qualified. Otherwise the definition would be a new global.

So it doesn't really have anything to do with static, except that normally our static declarations are global. MarkChildren is special because, again, there's a js::gc::MarkChildren signature declared in Marking.h.

This particular case is a little sad because exposing js::gc::MarkChildren in the header is a hack. I'd prefer to change the hack to use a different function name, and make all the MarkChildren signatures global and static.

> >  #include "mozilla/Assertions.h"
> I thought our normal include ordering put mozilla/ at the top?

Er, quite. Thanks. Fixed.

Comment 9

[Terrence Cole [:terrence]]

Oh dear, that /is/ extremely sad-making. Thank you for the explanation!  :-)

Comment 10

[Nicholas Nethercote [inactive]]

> So, apparently it's slightly preferred to do a qualified definition
> rather than wrap the whole .cpp file in namespace blocks

Can someone explain why?  If you don't wrap the whole .cpp file then all the local definitions (e.g. classes) will be in the global namespace rather than the |js| namespace.  This probably doesn't matter in practice, but it seems a bit inconsistent.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7600a1ab2d7c
https://hg.mozilla.org/mozilla-central/rev/d19500da9c16