Add ability to (de)serialize symbols #539
Conversation
|
@bnoordhuis Is this what you had in mind? If so, I'll add the missing part to serialize symbols directly, as in |
|
|
I believe so, I did consider it: The "problem" is that normal symbols won't match after being deserialized, but I'd say that is expected, right? |
|
Yes, I'd say so;
You're more or less handcrafting the symbol from the wire data now, right? Maybe it's easier to use |
|
I'm passing in the atom and the type so the distinction is already there. I'm only special casing the internal atoms. I'll continue tonight! |
saghul
force-pushed
the
write-symbols
branch
from
fe4287c to
804f588
Compare
|
@bnoordhuis Updated, PTAL! |
| #else | ||
| return (int32_t)v < JS_ATOM_END; | ||
| #endif | ||
| return (int32_t)v < JS_ATOM_END; |
There was a problem hiding this comment.
I'm not 100% sure this is ok, but I don't see what the previous check tries to accomplish. AFAICT, the only case in which (int32_t)v <= 0; will be true is for tagged integer atoms. But then, in JS_FreeRuntime we only check for atoms >= JS_ATOM_END or if the refcount is not 1, so I think we're good?
There was a problem hiding this comment.
I suspect it's intended as an extra reference counting stress test.
An atom that's a keyword, like Symbol("return"), is constant and exempt from reference counting - but being exempt means refcount bugs won't surface.
(Aside: I don't like the int32_t cast, too clever and relies on implementation-defined behavior.)
There was a problem hiding this comment.
I suspected something similar. I don't have a strong opinion, do you have a preference here?
There was a problem hiding this comment.
If it doesn't complicate this PR, I'd keep it; otherwise, chuck it.
There was a problem hiding this comment.
It doesn't complicate things much, but I see little value in it plus there is the all-too-clever cast necessary. Look me a while to figure out what was going on because the code behaved differently in debug mode. All that to say I'm chucking it. We can always reconsider down the road if we get bitten by it :-P
saghul
changed the title
| #else | ||
| return (int32_t)v < JS_ATOM_END; | ||
| #endif | ||
| return (int32_t)v < JS_ATOM_END; |
There was a problem hiding this comment.
I suspect it's intended as an extra reference counting stress test.
An atom that's a keyword, like Symbol("return"), is constant and exempt from reference counting - but being exempt means refcount bugs won't surface.
(Aside: I don't like the int32_t cast, too clever and relies on implementation-defined behavior.)
| "Map", | ||
| "Set", |
| JSAtom atom = s->idx_to_atom[i]; | ||
| if (__JS_AtomIsConst(atom)) { | ||
| bc_put_u8(s, 0 /* the type */); | ||
| bc_put_u32(s, atom); |
There was a problem hiding this comment.
The encoding for tagged integers could be more efficient, and for keyword-ish atoms (0 < atom < JS_ATOM_END) a single byte is enough, for now, because we only have 219.
Not so important however and fine to leave it for another time (but please add a TODO comment in that case.)
| if (bc_get_u32(s, &atom)) | ||
| return -1; | ||
| } else { | ||
| assert(type >= JS_ATOM_TYPE_STRING && type < JS_ATOM_TYPE_PRIVATE); |
There was a problem hiding this comment.
Since this reads untrusted input, it should return an error, not assert.
Fixes: #481