541452 - put jstracer.cpp in namespace js

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: do it

Most of the decls in jstracer.h/.cpp are unprefixed and yet globally exported (e.g., in jscntxt.h).  One way to achieve some semblance of cohesive style without adding a billion js_/JS prefixes is to move jstracer to the new SpiderMonkey C++ style of sticking things in namespace js {} instead of prefixing.  With this change, I stripped all remaining js_/JS prefixed in jstracer decls.

An unrelated change thrown in is to silence the func-ptr-to-data-ptr compiler warning that's been bugging me for a while.

Comment 1

[Luke Wagner [:luke]]

Attachment: missed a js_LeaveTrace

How could I forget dom/base/nsJSEnvironment.cpp?

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 423023 [details] [diff] [review]
missed a js_LeaveTrace

In jstracer.cpp, the "namespace nanojit { }" seems like it's probably
unnecessary. If so, please remove it.

The "namespace js { }" that covers the rest of the file might cause a problem because on ARM we include a bunch more headers right in the middle of jstracer.cpp. It's probably OK but who knows... I'm not too familiar with the ARM toolchain. I'm fine with (a) doing nothing, (b) moving those includes to the top, or (c) switching from `namespace js {}` to `using namespace js;`. I actually like (c) because I'm just that way, but your call.

Also:
> static bool
>+arm_check_thumb() {

While you're editing these lines, please put the curly brace on the following
line (house style for a long time). About 8 lines starting with "arm" can use
this tweak.

In jstracer.h:
>-enum JSTraceType_
>+enum TraceType
[...]
> #ifdef USE_TRACE_TYPE_ENUM
>-typedef JSTraceType_ JSTraceType;
>+typedef TraceType TraceType;
> #else
>-typedef int8_t JSTraceType;
>+typedef int8_t TraceType;
> #endif

Won't this change cause problems when USE_TRACE_TYPE_ENUM is not defined?

r=me with the nits addressed.

Comment 3

[Brendan Eich [:brendan]]

Why are there ARM-only nested includes in any .h file? Seems bogus on first principles (don't over-include, nest includes only to satisfy type size or similar such C++ requirements).

/be

Comment 4

[Luke Wagner [:luke]]

Thanks Jason; nice catch with the JSTraceType_ !  I totally whizzed right by it.

I'm going to go with your option (b).  The reason is that, to define a decl in a namespace, the identifier must be qualified if it is defined outside of the namespace.  E.g.:

// A.h
namespace N { void foo(); }

// A.cpp
void N::foo() {}

Simply defining an unqualified 'foo' in A.cpp will leave N::foo undefined.  So, less typing and less surprising linker errors (especially when we have so much conditional compilation).  I tend to reserve 'using namespace' for translation units primarily not defining namespace members.

Comment 5

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/d3e1459bc81c

Comment 6

[Nicholas Nethercote [inactive]]

Luke, in this line in jstracer.cpp:

  #if defined(NANOJIT_ARM) && defined(__GNUCC__) && defined(AVMPLUS_LINUX)

I think you want __GNUC__ instead of __GNUCC__.  Changing that fixes some bustage in my ARM VM.

Comment 7

[Luke Wagner [:luke]]

Apologies for not fixing this earlier!

http://hg.mozilla.org/tracemonkey/rev/887941de6c21

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/d3e1459bc81c