Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
mozilla14
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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)

Comment 1

5 years ago
Created attachment 604478 [details] [diff] [review]
Minor cleanups to atom declarations and definitios, in preparation for conversion to a table.
Assignee: general → jimb
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 604479 [details] [diff] [review]
Use a table to declare atoms.
(Assignee)

Comment 3

5 years ago
I haven't tested this thoroughly; will r? once that's done.
(Assignee)

Updated

5 years ago
Depends on: 733461
(Assignee)

Updated

5 years ago
Blocks: 733461
No longer depends on: 733461
(Assignee)

Comment 4

5 years ago
Try server: https://tbpl.mozilla.org/?tree=Try&rev=f6cd89f80c51
(Assignee)

Comment 5

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

Comment 6

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

Okay, that looks better.
Attachment #604479 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Attachment #604478 - Flags: review?(jwalden+bmo)
Cool.
(Assignee)

Comment 8

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

Comment 11

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

Comment 12

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

Comment 13

5 years ago
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?
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a566527261d
Flags: in-testsuite+
Target Milestone: --- → mozilla14
(Assignee)

Comment 16

5 years ago
I think we need a bugzilla flag for puns, to change the background or something so people aren't caught by surprise.
https://hg.mozilla.org/mozilla-central/rev/6841f0601afd
https://hg.mozilla.org/mozilla-central/rev/5a566527261d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
<3
You need to log in before you can comment on or make changes to this bug.