Closed Bug 541452 Opened 11 years ago Closed 11 years ago

put jstracer.cpp in namespace js

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Attached patch do it (obsolete) — Splinter Review
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.
Attachment #423022 - Flags: review?(jorendorff)
Summary: put jstracer.cpp in namespace js {} → put jstracer.cpp in namespace js
How could I forget dom/base/nsJSEnvironment.cpp?
Attachment #423022 - Attachment is obsolete: true
Attachment #423023 - Flags: review?(jorendorff)
Attachment #423022 - Flags: review?(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.
Attachment #423023 - Flags: review?(jorendorff) → review+
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
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.
http://hg.mozilla.org/tracemonkey/rev/d3e1459bc81c
Whiteboard: fixed-in-tracemonkey
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.
Apologies for not fixing this earlier!

http://hg.mozilla.org/tracemonkey/rev/887941de6c21
http://hg.mozilla.org/mozilla-central/rev/d3e1459bc81c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 546491
You need to log in before you can comment on or make changes to this bug.