666316 - Add jsdeprecated.h header

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Paul Biggar]

Attachment: Done except build system stuff

Some of the bugs from 666042 would completely screw embedders on the next spidermonkey release. For example, if they use JSFloat64, bug 666049 would break things for them.

To be less unpleasant, and also to give us a little more freedom in what we deprecate, this patch adds a jsdeprecate.h header, to which we can move our deprecated typedefs and #defines. jsdeprecate.h is included from jsapi.h and installed.

To ensure we aren't using deprecated types in Mozilla code, luke and I discusses a --disable-deprecated header. With that option, jsdeprecate.h is not included by jsapi.h, and is not installed.


Since it's a (pleasant) change in our contract with embedders, asking for superreview (I'll ask for a separate review on the build system integration, since it's not quite done).

Comment 1

[Paul Biggar]

Attachment: Done

Fixed build system stuff, thanks khuey!

Comment 2

[David Mandelin [:dmandelin]]

Comment on attachment 541132 [details] [diff] [review]
Done

I think it makes sense to get review from a separate person.

Comment 3

[Paul Biggar]

Just thinking maybe I should add a #warning in jsdeprecate.h, something like:

"Compiled using deprecated interfaces; configure with --disable-deprecated for hard errors."

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 541132 [details] [diff] [review]
Done

Since we last talked about this, the question came to mind: if the purpose of jsdeprecated.h is just for embedders using old typedefs/names we remove, it seems like perhaps we could just make the breaking change since its pretty easy to build a shim header (like Wes has done for various simple breaking changes we've made in the past).  In other words, I'm not sure if we're providing a great value here that isn't easy for embedders to do themselves (maybe I'm missing an angle, though) and, if we're not providing a great value, then it seems best to not do it.

Either way, I think sr? gets to decide the general question and r? would deal with your specific changes to the build system (since thats the meat of the patch) in which case I'm distinctly under-qualified.  Perhaps ted?

Comment 5

[Paul Biggar]

(In reply to comment #4)
> Comment on attachment 541132 [details] [diff] [review] [review]
> Done
> 
> Since we last talked about this, the question came to mind: if the purpose
> of jsdeprecated.h is just for embedders using old typedefs/names we remove,
> it seems like perhaps we could just make the breaking change since its
> pretty easy to build a shim header (like Wes has done for various simple
> breaking changes we've made in the past).  In other words, I'm not sure if
> we're providing a great value here that isn't easy for embedders to do
> themselves (maybe I'm missing an angle, though) and, if we're not providing
> a great value, then it seems best to not do it.

jsdeprecated.h is that shim. I think that providing it ourselves is the path of least pain, especially since it's relatively easy to do. We could just delete all this legacy stuff, but we'd cause embedders much pain, and that would make us more conservative than I'd like to be.


> Either way, I think sr? gets to decide the general question and r? would
> deal with your specific changes to the build system (since thats the meat of
> the patch) in which case I'm distinctly under-qualified.  Perhaps ted?

Cool. I'll ask khuey, since he's already looked at it.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 541132 [details] [diff] [review]
Done

> 
> dnl ========================================================
>+dnl = Deprecated interfaces
>+dnl ========================================================
>+JS_DEPRECATED=1
>+MOZ_ARG_DISABLE_BOOL(deprecated,
>+[  --disable-deprecated    Disable deprecated headers (default=no)],
>+    [JS_DEPRECATED=])

Add ,\n[JS_DEPCREATED=1]) to be consistent with other options.

>+if test "$JS_DEPRECATED"; then
>+    AC_DEFINE(JS_DEPRECATED)
>+    AC_SUBST(JS_DEPRECATED)
>+fi

You need to move the AC_SUBST out of the conditional.  We discussed this on IRC ...

Comment 7

[David Mandelin [:dmandelin]]

We're not going to do this right now.