Closed Bug 501536 Opened 12 years ago Closed 9 years ago

Convert jsd to C++

Categories

(Core :: JavaScript Engine, enhancement)

x86
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: luke, Assigned: terrence)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Attached patch draft (obsolete) — Splinter Review
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.
Assignee: nobody → timeless
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #386218 - Flags: review?
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
Attachment #386218 - Flags: review? → review?(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
Attachment #386218 - Flags: review?(brendan)
Component: JavaScript Debugging/Profiling APIs → JavaScript Engine
Blocks: 773686
Attached patch v0Splinter Review
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
Assignee: timeless → terrence
Attachment #386218 - Attachment is obsolete: true
Attachment #657481 - Flags: review?(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>
Attachment #657481 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/24298fb50710
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.