put jstracer.cpp in namespace js

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Created attachment 423022 [details] [diff] [review]
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.
Attachment #423022 - Flags: review?(jorendorff)
(Assignee)

Updated

9 years ago
Summary: put jstracer.cpp in namespace js {} → put jstracer.cpp in namespace js
(Assignee)

Comment 1

9 years ago
Created attachment 423023 [details] [diff] [review]
missed a js_LeaveTrace

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
(Assignee)

Comment 4

9 years ago
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.
(Assignee)

Comment 5

9 years ago
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.
(Assignee)

Comment 7

9 years ago
Apologies for not fixing this earlier!

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

Comment 8

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