Bug 162808 - JSStringRef should define JSChar without platform checks
Summary: JSStringRef should define JSChar without platform checks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-30 14:17 PDT by Nicolas Breidinger
Modified: 2016-10-03 10:35 PDT (History)
6 users (show)

See Also:


Attachments
JSStringRef change (1.10 KB, patch)
2016-09-30 14:26 PDT, Nicolas Breidinger
no flags Details | Formatted Diff | Diff
updated patch (1.17 KB, patch)
2016-09-30 16:52 PDT, Nicolas Breidinger
ggaren: review+
Details | Formatted Diff | Diff
updated patch (1.24 KB, patch)
2016-09-30 19:50 PDT, Nicolas Breidinger
no flags Details | Formatted Diff | Diff
updated patch (1.27 KB, patch)
2016-09-30 20:09 PDT, Nicolas Breidinger
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nicolas Breidinger 2016-09-30 14:17:08 PDT
JSChar is typedef'd as either "unsigned short" or "wchar_t" based on a number of platform checks. This is problematic for non-standard ports. To solve this, a way to determine which typedef to use without platform checks is required.
Comment 1 Nicolas Breidinger 2016-09-30 14:26:16 PDT
Created attachment 290384 [details]
JSStringRef change
Comment 2 Geoffrey Garen 2016-09-30 15:57:17 PDT
Comment on attachment 290384 [details]
JSStringRef change

View in context: https://bugs.webkit.org/attachment.cgi?id=290384&action=review

> Source/JavaScriptCore/API/JSStringRef.h:40
> +#if __WCHAR_MAX__ > 0x10000

Don't we need to check if __WCHAR_MAX__ is #defined?
Comment 3 Nicolas Breidinger 2016-09-30 16:39:16 PDT
(In reply to comment #2)
> Don't we need to check if __WCHAR_MAX__ is #defined?

Looking more closely at gcc and msvc, a better check would be as follows:
> #if !defined(_NATIVE_WCHAR_T_DEFINED) && (!defined(__WCHAR_MAX__) || (__WCHAR_MAX__ > 0xffffU))

_NATIVE_WCHAR_T_DEFINED being msvc specific
__WCHAR_MAX__ being gcc specific

However, I'm not familiar with mac implementation, it seems according to apple's libc (https://opensource.apple.com/source/Libc/Libc-825.24/include/_types.h) it is not guaranteed to exist:
> #ifdef __WCHAR_MAX__
> #define __DARWIN_WCHAR_MAX	__WCHAR_MAX__
> #else /* ! __WCHAR_MAX__ */
> #define __DARWIN_WCHAR_MAX	0x7fffffff
> #endif /* __WCHAR_MAX__ */
Comment 4 Nicolas Breidinger 2016-09-30 16:52:55 PDT
Created attachment 290405 [details]
updated patch
Comment 5 Geoffrey Garen 2016-09-30 17:19:16 PDT
Comment on attachment 290405 [details]
updated patch

r=me
Comment 6 Nicolas Breidinger 2016-09-30 19:50:29 PDT
Created attachment 290419 [details]
updated patch

Covers a missing case for RVCT.
Adds description for each set of compiler options.
Comment 7 Nicolas Breidinger 2016-09-30 20:09:51 PDT
Created attachment 290420 [details]
updated patch
Comment 8 Mark Lam 2016-10-03 08:51:41 PDT
Comment on attachment 290420 [details]
updated patch

r=me
Comment 9 WebKit Commit Bot 2016-10-03 09:45:30 PDT
Comment on attachment 290420 [details]
updated patch

Clearing flags on attachment: 290420

Committed r206734: <http://trac.webkit.org/changeset/206734>
Comment 10 WebKit Commit Bot 2016-10-03 09:45:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Konstantin Tokarev 2016-10-03 09:46:58 PDT
ChangeLog entry does not match patch contents!
Comment 12 Mark Lam 2016-10-03 10:35:55 PDT
(In reply to comment #11)
> ChangeLog entry does not match patch contents!

How so?