Last Comment Bug 734454 - One should be able to define a new atom without changing four different pieces of code
: One should be able to define a new atom without changing four different piece...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Jim Blandy :jimb
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 733461
  Show dependency treegraph
 
Reported: 2012-03-09 11:38 PST by Jim Blandy :jimb
Modified: 2012-03-18 12:07 PDT (History)
5 users (show)
jimb: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Minor cleanups to atom declarations and definitios, in preparation for conversion to a table. (17.74 KB, patch)
2012-03-09 11:39 PST, Jim Blandy :jimb
jwalden+bmo: review+
Details | Diff | Splinter Review
Use a table to declare atoms. (21.40 KB, patch)
2012-03-09 11:39 PST, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
try: -b do -n -p all -u all -t none (24.50 KB, patch)
2012-03-13 16:15 PDT, Jim Blandy :jimb
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Jim Blandy :jimb 2012-03-09 11:38:07 PST
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 2012-03-09 11:39:06 PST
Created attachment 604478 [details] [diff] [review]
Minor cleanups to atom declarations and definitios, in preparation for conversion to a table.
Comment 2 Jim Blandy :jimb 2012-03-09 11:39:47 PST
Created attachment 604479 [details] [diff] [review]
Use a table to declare atoms.
Comment 3 Jim Blandy :jimb 2012-03-09 11:40:17 PST
I haven't tested this thoroughly; will r? once that's done.
Comment 4 Jim Blandy :jimb 2012-03-10 17:41:39 PST
Try server: https://tbpl.mozilla.org/?tree=Try&rev=f6cd89f80c51
Comment 5 Jim Blandy :jimb 2012-03-12 10:39:32 PDT
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 2012-03-12 16:57:58 PDT
Comment on attachment 604479 [details] [diff] [review]
Use a table to declare atoms.

Okay, that looks better.
Comment 7 Nicholas Nethercote [:njn] 2012-03-13 02:12:57 PDT
Cool.
Comment 8 Jim Blandy :jimb 2012-03-13 16:15:44 PDT
Created attachment 605584 [details] [diff] [review]
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] (remove +bmo to email) 2012-03-15 17:42:23 PDT
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] (remove +bmo to email) 2012-03-15 17:46:53 PDT
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 2012-03-16 15:19:31 PDT
(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 2012-03-16 15:20:16 PDT
Thanks for the reviews, Jeff! I'll make the changes you suggest.
Comment 13 Jim Blandy :jimb 2012-03-16 16:03:03 PDT
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] (remove +bmo to email) 2012-03-16 16:06:14 PDT
(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 16 Jim Blandy :jimb 2012-03-16 16:19:03 PDT
I think we need a bugzilla flag for puns, to change the background or something so people aren't caught by surprise.
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2012-03-18 12:07:08 PDT
<3

Note You need to log in before you can comment on or make changes to this bug.