Convert jsd to C++

RESOLVED FIXED in mozilla18

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: luke, Assigned: terrence)

Tracking

unspecified
mozilla18
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v0
15.37 KB, patch
luke
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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

9 years ago
Created attachment 386218 [details] [diff] [review]
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.
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

Updated

8 years ago
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
Product: Core → Core
(Assignee)

Updated

6 years ago
Blocks: 773686
(Assignee)

Comment 4

6 years ago
Created attachment 657481 [details] [diff] [review]
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
Assignee: timeless → terrence
Attachment #386218 - Attachment is obsolete: true
Attachment #657481 - Flags: review?(luke)
(Reporter)

Comment 5

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.