Closed Bug 734454 Opened 9 years ago Closed 9 years ago

One should be able to define a new atom without changing four different pieces of code

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: jimb, Assigned: jimb)

References

Details

Attachments

(2 files, 1 obsolete file)

At present, to define a new atom in JSRuntime::atomState, one needs to:
1) add a member to JSAtomState
2) add an extern declaration for the js_foo_str to jsatom.h
3) define js_foo_str in jsatom.cpp, and
4) add an entry to js_common_atom_names.

This is a little bit annoying.
Assignee: general → jimb
Status: NEW → ASSIGNED
Attached patch Use a table to declare atoms. (obsolete) — Splinter Review
I haven't tested this thoroughly; will r? once that's done.
Depends on: 733461
Blocks: 733461
No longer depends on: 733461
Well, that was sobering. If at first you don't succeed, try, try again:
https://tbpl.mozilla.org/?tree=Try&rev=45b1e1fbe14d
Comment on attachment 604479 [details] [diff] [review]
Use a table to declare atoms.

Okay, that looks better.
Attachment #604479 - Flags: review?(jwalden+bmo)
Attachment #604478 - Flags: review?(jwalden+bmo)
As before, but added boilerplate and comments to jsatoms.tbl.
Attachment #604479 - Attachment is obsolete: true
Attachment #605584 - Flags: review?(jwalden+bmo)
Attachment #604479 - Flags: review?(jwalden+bmo)
Comment on attachment 604478 [details] [diff] [review]
Minor cleanups to atom declarations and definitios, in preparation for conversion to a table.

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

So, actually, I'd much rather we moved in the direction of not using named const char[] for all these, and instead just used string literals directly -- let the linker or whatever handle commoning up things.  But if you've already done this, and presumably relied on it in subsequent patches in this bug, I'm not going to stand in your way.  Although I'd really like us to move in that direction someday soon...
Attachment #604478 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 605584 [details] [diff] [review]
try: -b do -n -p all -u all -t none

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

::: js/src/jsatom.cpp
@@ +124,5 @@
>  #undef JS_PROTO
>  
> +#define DEFINE_ATOM(id, text)          ,js_ ## id ## _str
> +#define DEFINE_PROTOTYPE_ATOM(id)      ,js_ ## id ## _str
> +#define DEFINE_KEYWORD_ATOM(id)        ,js_ ## id ## _str

I think it's more common style in SpiderMonkey for ## to not be surrounded by spaces (perhaps so it reads more like a single token visually?).  This applies throughout your changes.

::: js/src/jsatom.tbl
@@ +33,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

Use the new MPL2 header.

@@ +155,5 @@
> +DEFINE_KEYWORD_ATOM(throw)
> +
> +#undef DEFINE_ATOM
> +#undef DEFINE_PROTOTYPE_ATOM
> +#undef DEFINE_KEYWORD_ATOM

Cute, having the header clean up after its user like that.  However, I don't think any of the other table files work this way.  Either do it the way all the others do, or change all the others to work this way as well.  (You could make those changes to all the others, should you choose to go that route, in a followup patch.)
Attachment #605584 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #9)
> So, actually, I'd much rather we moved in the direction of not using named
> const char[] for all these, and instead just used string literals directly
> -- let the linker or whatever handle commoning up things.  But if you've
> already done this, and presumably relied on it in subsequent patches in this
> bug, I'm not going to stand in your way.  Although I'd really like us to
> move in that direction someday soon...

Yeah, I checked that the linkers on all our platforms actually do this, and was about to undertake it... until I looked at all the uses of js_foo_str in the sources. Sometime when I'm up late but don't want to go to sleep, I can take that on.
Assignee: jimb → jhpedemonte
Component: JavaScript Engine → Java to XPCOM Bridge
QA Contact: general → xpcom-bridge
Thanks for the reviews, Jeff! I'll make the changes you suggest.
Assignee: jhpedemonte → jimb
Component: Java to XPCOM Bridge → JavaScript Engine
QA Contact: xpcom-bridge → general
Actually, I made a minor revision to this patch that allows us to use Java's "equal string literals are eq" semantics in C++, via XPCOM, to implement SpiderMonkey's atoms, so "Java to XPCOM Bridge" is now the correct component for this bug.
(In reply to Jim Blandy :jimb from comment #13)
> Actually, I made a minor revision to this patch that allows us to use Java's
> "equal string literals are eq" semantics in C++, via XPCOM, to implement
> SpiderMonkey's atoms, so "Java to XPCOM Bridge" is now the correct component
> for this bug.

Why are you wasting your time doing that?  Don't we have an intern to make equal strings eq for us?
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a566527261d
Flags: in-testsuite+
Target Milestone: --- → mozilla14
I think we need a bugzilla flag for puns, to change the background or something so people aren't caught by surprise.
You need to log in before you can comment on or make changes to this bug.