Closed Bug 541452 Opened 11 years ago Closed 11 years ago

put jstracer.cpp in namespace js


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Assigned: luke)



(Whiteboard: fixed-in-tracemonkey)


(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.

> 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
>-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).

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.
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!
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.