507458 - clear the waters a bit regarding JSVAL_BOOLEAN

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

So, as background, JSVAL_BOOLEAN is [currently] the tag for true, false, and undefined (a.k.a JSVAL_VOID).  That is, JSVAL_TAG(v) for those three values returns JSVAL_BOOLEAN.  However, JSVAL_IS_BOOLEAN(v) only returns true when v is true or false, not undefined.  Going the other direction, one uses PSEUDO_BOOLEAN_TO_JSVAL to inject {true,false,unspecified} into a jsval, BOOLEAN_TO_JSVAL only allows injecting {true,false}.

Clearly, this results in some confusion (which recently got me).  Case and point, jstracer.cpp redefines JSVAL_IS_BOOLEAN to assert(false) and has a cautionary comment.  Perhaps the nail in the coffin for me is that I wanted to write a function JSVAL_TO_TAG_ENUM which returned a value that could be used to switch (instead of the laddered if/else if/else necessary because JSVAL_INT) and, in the case of JSVAL_VOID, I have to either lie, and say JSVAL_VOID is a boolean (which JSVAL_IS_BOOLEAN says its not) or return some other value (which?).

So, what I propose is a tiny change.  Rename JSVAL_BOOLEAN to JSVAL_PSEUDO_BOOLEAN, add a JSVAL_IS_PSEUDO_BOOLEAN which just checks JSVAL_TAG(v) == JSVAL_PSEUDO_BOOLEAN, and do s/JSVAL_BOOLEAN/JSVAL_PSEUDO_BOOLEAN/ as needed throughout the codebase.

My simple grep shows that there are not many uses of booleans (mostly in jstracer which, as pointed out above, is already on shaky ground with the meaning of "BOOLEAN").  OTOH, this is definitely part of the public API and would break things.  OTOOH, its a simple change for users and would probably bring to light pre-existing misuses of booleans.

Am I missing anything?  Thoughts?

Comment 1

[Brendan Eich [:brendan]]

Sounds good -- the public API does not have pseudo-booleans in any released SpiderMonkey, and the latest jsapi.h has words and code trying to plug the obvious holes in the leaky abstraction.

Just don't change the names or semantics or values of JSVAL_VOID, JSVAL_IS_VOID, JSVAL_TRUE, JSVAL_FALSE, JSVAL_IS_BOOLEAN, JSVAL_TO_BOOLEAN, BOOLEAN_TO_JSVAL.

/be

Comment 2

[Jeff Walden [:Waldo]]

I was anti until I verified the first part of the first sentence of comment 1, a fact which I had not expected.  Go for it!

Comment 3

[Luke Wagner [:luke]]

Attachment: changes as proposed

Pretty simple.

Comment 4

[Brendan Eich [:brendan]]

Sorry, I'm guilty of not thinking about this harder. The tag is used for true and false (PSEUDO) booleans. It's not obvious what to call this tag, but using only the latter falsy name is misleading. JSVAL_TRUE and JSVAL_FALSE are in no sense "pseudo".

Perhaps we should avoid overlong names and use JSVAL_SPECIAL. Then we have room for more values (e.g., pointers tagged by JSVAL_SPECIAL and easily distinguished from small scalar values shifted over and so tagged).

/be

Comment 5

[Luke Wagner [:luke]]

Let's be more explicit; how about an acronym for: "Special, May Use Regarding Falseness".

s/JSVAL_PSEUDO_BOOLEAN/SMURF/g

Comment 6

[Brendan Eich [:brendan]]

dmandelin would love it.

But I was serious about JSVAL_SPECIAL.

/be

Comment 7

[Luke Wagner [:luke]]

Attachment: s/PSEUDO_BOOLEAN/JSVAL_SPECIAL/g

I left TT_PSEUDOBOOLEAN as is since it describes a 3-value set while JSVAL_SPECIAL (old JSVAL_BOOLEAN) describes the set of 29-bit tagged values.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 392623 [details] [diff] [review]
s/PSEUDO_BOOLEAN/JSVAL_SPECIAL/g

> typedef enum jsvaltag {
>     JSVAL_OBJECT  =         0x0,     /* untagged reference to object */
>     JSVAL_INT     =         0x1,     /* tagged 31-bit integer value */
>     JSVAL_DOUBLE  =         0x2,     /* tagged reference to double */
>     JSVAL_STRING  =         0x4,     /* tagged reference to string */
>-    JSVAL_BOOLEAN =         0x6      /* tagged boolean value */
>+    JSVAL_SPECIAL =         0x6      /* tagged boolean or private value */
> } jsvaltag;
> 
> #define JSVAL_OBJECT ((jsvaltag)0x0)
> #define JSVAL_INT ((jsvaltag)0x1)
> #define JSVAL_DOUBLE ((jsvaltag)0x2)
> #define JSVAL_STRING ((jsvaltag)0x4)
>-#define JSVAL_BOOLEAN ((jsvaltag)0x6)
>+#define JSVAL_PRIVATE ((jsvaltag)0x6)

s/PRIVATE/SPECIAL on that last line -- looks like a unique mistransliteration error.

I forgot why we have enum and #define names, the latter trumping the former, for the JSVAL_* tag bits. Some C++ goodness? But the macros hide all.

Nit (belated): indent the ((jsvaltag)0xN) bodies to start in the same column. This will help justify the somewhat strung-out 0x0, 0x1, etc. gratuitously hex tag values in the enum :-P.

>  * A JSVAL_HOLE can be cheaply converted to undefined without affecting any
>- * other boolean (or pseudo boolean) by masking out JSVAL_HOLE_MASK.
>+ * other boolean (or special value) by masking out JSVAL_HOLE_MASK.
>  *
>  * JSVAL_ARETURN is used to throw asynchronous return for generator.close().
>  *
>- * NB: PSEUDO_BOOLEAN_TO_JSVAL(2) is JSVAL_VOID (see jsapi.h).
>+ * NB: SPECIAL_TO_JSVAL(2) is JSVAL_VOID (see jsapi.h).
>  */
> #define JSVAL_HOLE_FLAG jsval(4 << JSVAL_TAGBITS)

Pre-existing comment typo: s/JSVAL_HOLE_MASK/JSVAL_HOLE_FLAG/g

> /*
>  * Never use JSVAL_IS_BOOLEAN because it restricts the value (true, false) and
>- * the type. What you want to use is JSVAL_TAG(x) == JSVAL_BOOLEAN and then
>+ * the type. What you want to use is JSVAL_IS_SPECIAL and then
>  * handle the undefined case properly (bug 457363).

Nit: rewrap to make right margin prettier. Qj in vim ;-).

>  */
> #undef JSVAL_IS_BOOLEAN
> #define JSVAL_IS_BOOLEAN(x) JS_STATIC_ASSERT(0)

Do we need this any longer? Probably.

>       case TT_PSEUDOBOOLEAN:

TT_SPECIAL now, right?

/be

Comment 9

[Luke Wagner [:luke]]

(In reply to comment #8)
> I forgot why we have enum and #define names, the latter trumping the former,
> for the JSVAL_* tag bits. Some C++ goodness?

I was going to the same question except s/C++/old C compiler/.  From a C++ POV, its seems strictly worse.  With an enumeration you can issue better warnings and, I believe, do better switch generation.  As one would expect, taking out the #defines compiles fine.  Can I just remove 'em?

> >       case TT_PSEUDOBOOLEAN:
> 
> TT_SPECIAL now, right?

From my previous comment, I was thinking TT_PSEUDOBOOLEAN should remain because it describes a 3-element set of jsvals, whereas JSVAL_SPECIAL represents a 2^29-element set of jsvals.  OTOH, if we are tired of thinking/typing PSEUDOBOOLEAN, a more appropriate name might be TT_3VL or TT_TRIBOOL since {true,false,undefined} is superfically similar to 3-valued logic.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> (In reply to comment #8)
> > I forgot why we have enum and #define names, the latter trumping the former,
> > for the JSVAL_* tag bits. Some C++ goodness?
> 
> I was going to the same question except s/C++/old C compiler/.  From a C++ POV,
> its seems strictly worse.  With an enumeration you can issue better warnings
> and, I believe, do better switch generation.  As one would expect, taking out
> the #defines compiles fine.  Can I just remove 'em?

They were added intentionally in the patch for bug 496908, presumably cuz I worried some old jsapi.h user was testing #ifdef JSVAL_OBJECT or some such. But I didn't spot them and I think my worry should be assuaged by trying to move to enum and listening for cries of pain. It's not hard to change an #ifdef that arguably shouldn't have been testing one of these names assuming it was a macro.
 
> > >       case TT_PSEUDOBOOLEAN:
> > 
> > TT_SPECIAL now, right?
> 
> From my previous comment, I was thinking TT_PSEUDOBOOLEAN should remain because
> it describes a 3-element set of jsvals, whereas JSVAL_SPECIAL represents a
> 2^29-element set of jsvals.  OTOH, if we are tired of thinking/typing
> PSEUDOBOOLEAN, a more appropriate name might be TT_3VL or TT_TRIBOOL since
> {true,false,undefined} is superfically similar to 3-valued logic.

Waldo or you (I forget who) suggested QUASI- as better than PSEUDO when two of the three values is a true boolean. Length is not such a problem but accuracy is with me. Don't know about TRIBOOL, not bad (SQL has something like that), it is a novelty, though. CRAZYBOOLEAN?

/be

Comment 11

[Jeff Walden [:Waldo]]

None of tribool, crazyboolean, quasiboolean, etc. seem better enough than pseudo to me to warrant expending effort to make a change, nor strike me as particularly desirable given the much smaller state space of true/false/undefined versus 2**29ish values.

Comment 12

[Brendan Eich [:brendan]]

Pseudo- meaning false is fair if you think any pseudo- along with the real true and false values poisons the "well of truth".

But this is not a big deal, and since we do not have a clearly better name, it seems fine to stick at TT_PSEUDOBOOLEAN.

/be

Comment 13

[Brendan Eich [:brendan]]

New patch losing the JSVAL_OBJECT, etc. #defines?

/be

Comment 14

[Luke Wagner [:luke]]

Attachment: tweaked and rebased

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 394070 [details] [diff] [review]
tweaked and rebased

Yay, go.

/be

Comment 16

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/0452549eecaf

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0452549eecaf

Comment 18

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a9782502676a