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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

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.

Comment 1

[Jim Blandy :jimb]

Attachment: Minor cleanups to atom declarations and definitios, in preparation for conversion to a table.

Comment 2

[Jim Blandy :jimb]

Attachment: Use a table to declare atoms.

Comment 3

[Jim Blandy :jimb]

I haven't tested this thoroughly; will r? once that's done.

Comment 4

[Jim Blandy :jimb]

Try server: https://tbpl.mozilla.org/?tree=Try&rev=f6cd89f80c51

Comment 5

[Jim Blandy :jimb]

Well, that was sobering. If at first you don't succeed, try, try again:
https://tbpl.mozilla.org/?tree=Try&rev=45b1e1fbe14d

Comment 6

[Jim Blandy :jimb]

Comment on attachment 604479 [details] [diff] [review]
Use a table to declare atoms.

Okay, that looks better.

Comment 7

[Nicholas Nethercote [inactive]]

Cool.

Comment 8

[Jim Blandy :jimb]

Attachment: try: -b do -n -p all -u all -t none

As before, but added boilerplate and comments to jsatoms.tbl.

Comment 9

[Jeff Walden [:Waldo]]

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

Comment 10

[Jeff Walden [:Waldo]]

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

Comment 11

[Jim Blandy :jimb]

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

Comment 12

[Jim Blandy :jimb]

Thanks for the reviews, Jeff! I'll make the changes you suggest.

Comment 13

[Jim Blandy :jimb]

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.

Comment 14

[Jeff Walden [:Waldo]]

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

Comment 15

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a566527261d

Comment 16

[Jim Blandy :jimb]

I think we need a bugzilla flag for puns, to change the background or something so people aren't caught by surprise.

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6841f0601afd
https://hg.mozilla.org/mozilla-central/rev/5a566527261d

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

<3