501536 - Convert jsd to C++

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Luke Wagner [:luke]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11) Gecko/2009060308 Ubuntu/9.04 (jaunty) Firefox/3.0.11
Build Identifier: 

As C++ template classes are used more, they will appear in JS headers and break jsd.  Furthermore, when compiling .cpp files in jsd, JS headers are wrapped with extern "C", which also breaks on templates.  Currently, there is a rather ghastly workaround in the patch to jsprvtd.h (bug 200505 comment 104).  This can be removed once jsd compiles as C++ and the extern "C" is removed from jsdebug.h.

Reproducible: Always

Comment 1

[timeless]

Attachment: draft

i'm not sure i agree with this. however, this at a baseline compiles

there's one random change here (some archeology should be done for jsd_SetContextPrivate).

and i'd like to test w/ jsdb to make sure it's still happy.

Comment 2

[Murali Nandigama [:murali]]

You have a patch on this bug that is flagged for 'review?' and not assigned to any reviewer. If you want the patch to be reviewed please assign a reviewer. Thanks

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 386218 [details] [diff] [review]
draft

>+CPPSRCS         = \
>+                  jsdebug.cpp \
>+                  jsd_atom.cpp \
>+                  jsd_high.cpp \
>+                  jsd_hook.cpp \
>+                  jsd_lock.cpp \
>+                  jsd_obj.cpp \
>+                  jsd_scpt.cpp \
>+                  jsd_stak.cpp \
>+                  jsd_step.cpp \
>+                  jsd_text.cpp \
>+                  jsd_val.cpp \
>+                  $(NULL)

Keep using tabs in Makefiles, they matter in rule bodies (not here but best to be always tab-expanding).

>+#define jsd_ISLOCKED(lock) jsd_IsLocked((JSDStaticLock*)lock)
>+#define jsd_LOCK(lock)     jsd_Lock((JSDStaticLock*)lock)
>+#define jsd_UNLOCK(lock)   jsd_Unlock((JSDStaticLock*)lock)
>+
> /* the system-wide lock */
> extern void* _jsd_global_lock;

Can't you use an opaque typename instead of void* and avoid the new casting macros?

I'm gonna invoke delegation rights and redirect rest of review at a C++ guru, Luke for instance. Please solicit r?lw@moz on my behalf next time.

/be

Comment 4

[Terrence Cole [:terrence]]

Attachment: v0

Not based on the existing patch: too trivial to bother.  This also fixes all build warnings, or at least the ones that showed up on my instance of g++.

https://tbpl.mozilla.org/?tree=Try&rev=84e713b7f050

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 657481 [details] [diff] [review]
v0

Review of attachment 657481 [details] [diff] [review]:
-----------------------------------------------------------------

Whoa, new world order, we can straight-up start using C++ in jsapi.h now.

::: js/jsd/jsd_lock.c
@@ +81,5 @@
>  jsd_CreateLock()
>  {
>      JSDStaticLock* lock;
>  
> +    if( ! (lock = static_cast<JSDStaticLock*>(calloc(1, sizeof(JSDStaticLock)))) || 

You can use pod_calloc<JSDStaticLock>() now :)

::: js/jsd/jsd_text.c
@@ +428,5 @@
>  
>      JSD_LOCK_SOURCE_TEXT(jsdc);
>      if(!buf)
>      {
> +        buf = static_cast<char*>(malloc(UNICODE_TRUNCATE_BUF_SIZE));

pod_malloc<char>

Comment 6

[Terrence Cole [:terrence]]

> pod_malloc<char>

Ah, very nice!

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=84e713b7f050

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24298fb50710

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/24298fb50710