Closed Bug 496908 Opened 11 years ago Closed 10 years ago

[FIX]Make JSVAL_IS_* functions, not macros

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

I saw a patch today using JSVAL_IS_NULL on a JSObject*.  Let's not have that compile.  ;)
Attached patch Like soSplinter Review
Assignee: jst → bzbarsky
Status: NEW → ASSIGNED
Attachment #382151 - Flags: review?(brendan)
Comment on attachment 382151 [details] [diff] [review]
Like so

No XXXbz in jsapi.h, at most a FIXME: bug nnnnnn. But let's hash this out first.

/be
Sure; I'm also happy to change those to functions and the asserts to runtime asserts.  Just let me know what you prefer.
Comment on attachment 382151 [details] [diff] [review]
Like so

>+typedef enum {
>+    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 */
>+} jsvaltag;

Drive-by comment: any reason to not just have

enum jsvaltag {
...
};

now that we're C++?
jsapi must still be usable from C code, no?
(In reply to comment #5)
> jsapi must still be usable from C code, no?

Right. C99 at this point, and we haven't user-tested the latest jsapi.h on the old embeddings out there.

/be
Comment on attachment 382151 [details] [diff] [review]
Like so

>-#define JSVAL_OBJECT            0x0     /* untagged reference to object */
>-#define JSVAL_INT               0x1     /* tagged 31-bit integer value */
>-#define JSVAL_DOUBLE            0x2     /* tagged reference to double */
>-#define JSVAL_STRING            0x4     /* tagged reference to string */
>-#define JSVAL_BOOLEAN           0x6     /* tagged boolean value */
>+typedef enum {
>+    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 */
>+} jsvaltag;

I wonder whether anyone does #ifdef JSVAL_OBJECT -- API is ancient, macros are discoverable. We'll find out.

Would typedef enum jsvaltag { ... } jsvaltag; work better for debuggers? I think anyone using C code would benefit from the enum having a name, from gdb past-life memory.

>+/* Not a function, because we have static asserts that use it */
>+#define JSVAL_TAG(v)            ((jsvaltag)((v) & JSVAL_TAGMASK))
>+/* Not a function, because we have static asserts that use it */
>+#define JSVAL_SETTAG(v, t) ((v) | (t))

Blank line before any comment taking one or more lines by itself, unless the preceding non-whitespace char is {.

> #define JSVAL_INT_MAX           (JSVAL_INT_POW2(30) - 1)
>+/* Not a function, because we have static asserts that use it */
> #define INT_FITS_IN_JSVAL(i)    ((jsuint)(i) - (jsuint)JSVAL_INT_MIN <=      \
>                                  (jsuint)(JSVAL_INT_MAX - JSVAL_INT_MIN))
>+/* Not a function, because we have static asserts that use it */
>+/* XXXbz that means we can't assert JSVAL_IS_INT(v) */
> #define JSVAL_TO_INT(v)         ((jsint)(v) >> 1)
>+/* Not a function, because we have static asserts that use it */
>+/* XXXbz that means we can't assert INT_FITS_IN_JSVAL(i) */
> #define INT_TO_JSVAL(i)         (((jsval)(i) << 1) | JSVAL_INT)

Ditto above about blank lines.

More imp., don't leave XXXbz behind if there's no way to fix this and you're in a public API. In general FIXME: bug nnnnnnn is the way to go, but here if there's no solution then there's no problem!

/be
(In reply to comment #7)
> I wonder whether anyone does #ifdef JSVAL_OBJECT -- API is ancient, macros are
> discoverable. We'll find out.

We could have the enum, then have #defines after the enum definition to head it off at the pass.


> Would typedef enum jsvaltag { ... } jsvaltag; work better for debuggers? I
> think anyone using C code would benefit from the enum having a name, from gdb
> past-life memory.

Seems reasonable.


> More imp., don't leave XXXbz behind if there's no way to fix this and you're in
> a public API. In general FIXME: bug nnnnnnn is the way to go, but here if
> there's no solution then there's no problem!

I will tentatively dispute the antecedent, because I think C++0x constexpr might be able to save the day here, possibly -- I know only what Wikipedia tells me, but it might address this case.  In any case, I'm not willing to give up that easily.
C++0x will crap skittles, I'm sure. In the mean time, FIXME or silence :-/.

/be
> Would typedef enum jsvaltag { ... } jsvaltag; work better for debuggers?

No idea; I'm happy to do that, though.

> More imp., don't leave XXXbz behind if there's no way to fix this 

This was more of a question-for-the-reviewer.  Do we want to fix it?  I'm fine either filing a bug or just not commenting on the matter at all.  It's your call which you want.

Do you want to see a patch with the above comments addressed, or are you still reviewing?
(In reply to comment #10)
> > Would typedef enum jsvaltag { ... } jsvaltag; work better for debuggers?
> 
> No idea; I'm happy to do that, though.

Cool, please do.

> > More imp., don't leave XXXbz behind if there's no way to fix this 
> 
> This was more of a question-for-the-reviewer.  Do we want to fix it?  I'm fine
> either filing a bug or just not commenting on the matter at all.  It's your
> call which you want.
> 
> Do you want to see a patch with the above comments addressed, or are you still
> reviewing?

I was done, Waldo should have a look too. r=me with nits addressed, r? Waldo for ultimate best effect. Thanks,

/be
I'd love an answer to my question about the XXX comment too!
(In reply to comment #12)
> I'd love an answer to my question about the XXX comment too!

Didn't comment 8 and comment 9 handle this? Seems like a FIXME: bug nnnnnn where the new bug talks about the constexpr C++0x idea is the best we can do for now.

/be
The macro idea was a suggestion I doubt we actually need, I'm fine without it -- but worth pointing out now so it's known in case we do.  A FIXME comment seems fine, I personally don't mind the format as long as the problem is noted.  Consider this an r=me as well.
Filed bug 506721 as the FIXME bug.
Pushed http://hg.mozilla.org/tracemonkey/rev/6fc1700090b2
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/6fc1700090b2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 509354
Attachment #382151 - Flags: review?(brendan)
Comment on attachment 382151 [details] [diff] [review]
Like so

This landed, and then changed a bit, AFAICT. All good in the end.

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