Hi, during our work on making the Qt port of WebKit compile on Windows with MSVC and MingW g++ we ran into the following code in JavaScriptCore/API/JSStringRef.h: ... #if !defined(WIN32) && !defined(_WIN32) typedef unsigned short JSChar; #else typedef wchar_t JSChar; #endif ... JSChar being defined as wchar_t caused compiles problems for us inside WebCore itself when converting from JSChar to UChar. For now we added a || defined(__BUILDING_QT) to the condition, but we're wondering why JSChar is defined as wchar_t on Windows in the first place. We ran into this problem only when compiling with g++, MSVC seems to silently convert wchar_t to unsigned short. Unfortunately the svn history does not provide anything regard this #ifdef since it was added to the svn repository. Does anybody know/remember why JSChar is defined to wchar_t on Windows and if it is still needed? Simon
On 7/27/07 1:51 PM, "Simon Hausmann" <hausmann@kde.org> wrote:
Does anybody know/remember why JSChar is defined to wchar_t on Windows and if it is still needed?
I think this was/is needed to match ICU's definition of UChar ("Define UChar to be wchar_t if that is 16 bits wide; always assumed to be unsigned. If wchar_t is not 16 bits wide, then define UChar to be uint16_t. This makes the definition of UChar platform-dependent but allows direct string type compatibility with platforms with 16-bit wchar_t types.") - WBR, Alexey Proskuryakov
On Jul 27, 2007, at 4:03 AM, Alexey Proskuryakov wrote:
On 7/27/07 1:51 PM, "Simon Hausmann" <hausmann@kde.org> wrote:
Does anybody know/remember why JSChar is defined to wchar_t on Windows and if it is still needed?
I think this was/is needed to match ICU's definition of UChar ("Define UChar to be wchar_t if that is 16 bits wide; always assumed to be unsigned. If wchar_t is not 16 bits wide, then define UChar to be uint16_t. This makes the definition of UChar platform- dependent but allows direct string type compatibility with platforms with 16-bit wchar_t types.")
That's correct. And for the same reasons we should follow the same pattern for UChar, even when ICU is not involved. I think that UnicodeQt4.h is the file that should be changed, not JSStringRef.h. -- Darin
On Jul 27, 2007, at 7:45 AM, Darin Adler wrote:
On Jul 27, 2007, at 4:03 AM, Alexey Proskuryakov wrote:
On 7/27/07 1:51 PM, "Simon Hausmann" <hausmann@kde.org> wrote:
Does anybody know/remember why JSChar is defined to wchar_t on Windows and if it is still needed?
I think this was/is needed to match ICU's definition of UChar ("Define UChar to be wchar_t if that is 16 bits wide; always assumed to be unsigned. If wchar_t is not 16 bits wide, then define UChar to be uint16_t. This makes the definition of UChar platform-dependent but allows direct string type compatibility with platforms with 16-bit wchar_t types.")
That's correct. And for the same reasons we should follow the same pattern for UChar, even when ICU is not involved.
I think that UnicodeQt4.h is the file that should be changed, not JSStringRef.h.
I should be more specific about the reasons I have for preferring this. It's a good feature for both JSChar and UChar that on the Windows platform, the 16-bit unsigned integer type they use is wchar_t, rather than unsigned short. That's because there are all sorts of Windows Unicode APIs that take wchar_t, and it's great to have the type match. The same issue exists on Mac OS X, but on that platform the types for characters, Unichar and unichar, are unsigned short. Anyway, for the JavaScript API, it's great to have the character typedef match the platform typedef, as long as the platform typedef is a 16-bit unsigned integer. I'd like us to preserve that feature even when building for Qt. That increases the chance that client code using the JavaScript API can be independent of the particular underlying JavaScriptCore implementation. Also, since JSStringRef.h is an API header, it's not good to have it depend on an project-internal define, so that's another reason to remove the recent change to JSStringRef.h. -- Darin
On Friday 27 July 2007 18:56:40 Darin Adler wrote:
On Jul 27, 2007, at 7:45 AM, Darin Adler wrote:
On Jul 27, 2007, at 4:03 AM, Alexey Proskuryakov wrote:
On 7/27/07 1:51 PM, "Simon Hausmann" <hausmann@kde.org> wrote:
Does anybody know/remember why JSChar is defined to wchar_t on Windows and if it is still needed?
I think this was/is needed to match ICU's definition of UChar ("Define UChar to be wchar_t if that is 16 bits wide; always assumed to be unsigned. If wchar_t is not 16 bits wide, then define UChar to be uint16_t. This makes the definition of UChar platform-dependent but allows direct string type compatibility with platforms with 16-bit wchar_t types.")
That's correct. And for the same reasons we should follow the same pattern for UChar, even when ICU is not involved.
I think that UnicodeQt4.h is the file that should be changed, not JSStringRef.h.
I should be more specific about the reasons I have for preferring this.
It's a good feature for both JSChar and UChar that on the Windows platform, the 16-bit unsigned integer type they use is wchar_t, rather than unsigned short. That's because there are all sorts of Windows Unicode APIs that take wchar_t, and it's great to have the type match.
I do understand this for your Windows port. In Qt we try to have the same types across all platforms. This is also true for our QChar abstraction that is built on top of an unsigned short. So for us typedef'ing this to wchar_t is as wrong as unsigned short would be for your windows port.
The same issue exists on Mac OS X, but on that platform the types for characters, Unichar and unichar, are unsigned short.
Anyway, for the JavaScript API, it's great to have the character typedef match the platform typedef, as long as the platform typedef is a 16-bit unsigned integer. I'd like us to preserve that feature even when building for Qt. That increases the chance that client code using the JavaScript API can be independent of the particular underlying JavaScriptCore implementation.
For the Qt port, the 'platform' typedef is an unsigned short. So one could say that the #ifdef for __BULDING_QT does about the same thing as the #ifdef (WIN32) for your Windows port.
Also, since JSStringRef.h is an API header, it's not good to have it depend on an project-internal define, so that's another reason to remove the recent change to JSStringRef.h.
Well, it's not an API header for us. Still it's used internally in WebKit, so we need to do something with the typedef. Different ports do sometimes have slightly different goals. Cheers, Lars
On Jul 27, 2007, at 11:18 AM, Lars Knoll wrote:
I do understand this for your Windows port. In Qt we try to have the same types across all platforms. This is also true for our QChar abstraction that is built on top of an unsigned short. So for us typedef'ing this to wchar_t is as wrong as unsigned short would be for your windows port.
I'm sorry we don't see eye to eye on this. In the future there's good reason for this to be API. It was designed to be cross-platfom API without any reliance on any particular platform. I'd like to see this left as a possibility for Windows.
For the Qt port, the 'platform' typedef is an unsigned short. So one could say that the #ifdef for __BULDING_QT does about the same thing as the #ifdef (WIN32) for your Windows port.
OK. But would we expect people to define __BULDING_QT when using this header? We've gone out of our way to not use the internal definitions like Platform.h in these headers, designed to be API and in a subdirectory named "API", and it's strange to have __BUILDING_QT here.
Also, since JSStringRef.h is an API header, it's not good to have it depend on an project-internal define, so that's another reason to remove the recent change to JSStringRef.h.
Well, it's not an API header for us. Still it's used internally in WebKit, so we need to do something with the typedef. Different ports do sometimes have slightly different goals.
Longer term I'd like to see this as cross-platform API. Even though you don't see the need for this in your Qt port today, I think you might in the future, for example if it was used as part of a plug-in API and we wanted the plug-ins to work in both Qt WebKit on Windows and the WebKit on Windows that's released with Safari. Could you think about this a little more and see if you're swayed by my arguments? Also, please discuss this with Maciej if you have a chance. -- Darin
One last comment that might help. The idea here is that this is a low level API. Lower level than, say, the WebKit API. It's not built on top of the platform APIs like AppKit on Mac OS X. The idea is that it's potentially independent of WebKit. That's a good argument for having it match the low level platform and not be built on top of, say, AppKit or Qt. That extends even to things like this type. I guess the whole wchar_t thing is a snafu, though, because we normally would not want a type that is different on different platforms. Maybe we should dump the idea of using wchar_t on Windows for this. We were really following ICU's lead here -- ICU being another low level library not built on top of a framework like Qt or AppKit. -- Darin
I do understand this for your Windows port. In Qt we try to have the same types across all platforms. This is also true for our QChar abstraction that is built on top of an unsigned short. So for us typedef'ing this to wchar_t is as wrong as unsigned short would be for your windows port.
I'm sorry we don't see eye to eye on this. In the future there's good reason for this to be API. It was designed to be cross-platfom API without any reliance on any particular platform. I'd like to see this left as a possibility for Windows.
Could you explain what you mean by 'good reason'?
For the Qt port, the 'platform' typedef is an unsigned short. So one could say that the #ifdef for __BULDING_QT does about the same thing as the #ifdef (WIN32) for your Windows port.
OK. But would we expect people to define __BULDING_QT when using this header?
We are not planning to export this header as part of our public API. If someone want's to use this API together with JavascriptCore he is free to do so, but this is not at all related to a Qt port of WebKit.
We've gone out of our way to not use the internal definitions like Platform.h in these headers, designed to be API and in a subdirectory named "API", and it's strange to have __BUILDING_QT here.
I'd also prefer not having it in there. Still for Qt as a platform, the correct typedef is unsigned short.
Also, since JSStringRef.h is an API header, it's not good to have it depend on an project-internal define, so that's another reason to remove the recent change to JSStringRef.h.
Well, it's not an API header for us. Still it's used internally in WebKit, so we need to do something with the typedef. Different ports do sometimes have slightly different goals.
Longer term I'd like to see this as cross-platform API. Even though you don't see the need for this in your Qt port today, I think you might in the future, for example if it was used as part of a plug-in API and we wanted the plug-ins to work in both Qt WebKit on Windows and the WebKit on Windows that's released with Safari.
To have this usable by plugins it would have to be a cross browser API as well as a cross platform one. Are there any efforts on the way in this direction?
Could you think about this a little more and see if you're swayed by my arguments? Also, please discuss this with Maciej if you have a chance.
On Friday 27 July 2007 20:32:32 Darin Adler wrote:
One last comment that might help.
The idea here is that this is a low level API. Lower level than, say, the WebKit API. It's not built on top of the platform APIs like AppKit on Mac OS X. The idea is that it's potentially independent of WebKit. That's a good argument for having it match the low level platform and not be built on top of, say, AppKit or Qt. That extends even to things like this type.
If someone want's to use JavascriptCore with this low level API he is free to do so.
I guess the whole wchar_t thing is a snafu, though, because we normally would not want a type that is different on different platforms. Maybe we should dump the idea of using wchar_t on Windows for this.
Yes. If the API is supposed to be cross platform it would make most sense if the types are also compatible. wchar_t is unfortunately a pretty unusable type for cross platform API's as it is 16bit on Windows and 32bit on Linux.
We were really following ICU's lead here -- ICU being another low level library not built on top of a framework like Qt or AppKit.
I do see that. In Qt, although we have lot's of the same functionality as ICU builtin, we chose a different path and used unsigned short on all platforms (as it's 16bit on all platforms we support). Lars
On Jul 27, 2007, at 11:51 AM, Lars Knoll wrote:
Could you explain what you mean by 'good reason'?
A JavaScript engine API that isolates clients from implementation details and can be potentially used cross platform is valuable. The files in the API directory represent an attempt to create one. That's the only reason these exist at all!
I'd also prefer not having it in there. Still for Qt as a platform, the correct typedef is unsigned short.
This API should have nothing to do with Qt as a platform. Any use of Qt in the implementation should be a hidden detail.
To have this usable by plugins it would have to be a cross browser API as well as a cross platform one. Are there any efforts on the way in this direction?
Yes, I think there's potential for this as a cross-JavaScript-engine API and hence a cross-browser one. No, we don't have specific plans to make that happen yet.
We were really following ICU's lead here -- ICU being another low level library not built on top of a framework like Qt or AppKit.
I do see that. In Qt, although we have lot's of the same functionality as ICU built in, we chose a different path and used unsigned short on all platforms (as it's 16bit on all platforms we support).
Yes, and Qt also has its own JavaScript engine. I'd really like this API to be independent of the high level framework it's being used with, and I think it's unfortunate that Qt is now mentioned in the header. I'd prefer a different solution. -- Darin
On Jul 27, 2007, at 11:59 AM, Darin Adler wrote:
We were really following ICU's lead here -- ICU being another low level library not built on top of a framework like Qt or AppKit.
I do see that. In Qt, although we have lot's of the same functionality as ICU built in, we chose a different path and used unsigned short on all platforms (as it's 16bit on all platforms we support).
Yes, and Qt also has its own JavaScript engine.
I'd really like this API to be independent of the high level framework it's being used with, and I think it's unfortunate that Qt is now mentioned in the header. I'd prefer a different solution.
I haven't really thought about this one much but if there is to be an ifdef used in this file at all, it should ideally use a symbol that would be set when building a Qt-using application, not an internal platform define (even if this API isn't exported to Qt applications for now). Regards, Maciej
Darin Adler wrote:
On Jul 27, 2007, at 4:03 AM, Alexey Proskuryakov wrote:
On 7/27/07 1:51 PM, "Simon Hausmann" <hausmann@kde.org> wrote:
Does anybody know/remember why JSChar is defined to wchar_t on Windows and if it is still needed?
I think this was/is needed to match ICU's definition of UChar ("Define UChar to be wchar_t if that is 16 bits wide; always assumed to be unsigned. If wchar_t is not 16 bits wide, then define UChar to be uint16_t. This makes the definition of UChar platform-dependent but allows direct string type compatibility with platforms with 16-bit wchar_t types.")
That's correct. And for the same reasons we should follow the same pattern for UChar, even when ICU is not involved.
I think that UnicodeQt4.h is the file that should be changed, not JSStringRef.h.
You say WebKit tries to follow ICU's UChar. If so, the real size of wchar_t must be examined instead of just checking for WIN32 or _WIN32. Otherwise the problem appears for every windows/g++ build without these two defines. In the case of Qt (which doesn't use ICU), the problem can be easily solved: define UChar in UnicodeQt4.h according to ICU's UChar - see Simon's last email - but what about other platforms that don't have their own Unicode implementation? Thanks, Artem
On Friday 27 July 2007 11:51:40 Simon Hausmann wrote: [...]
JavaScriptCore/API/JSStringRef.h:
... #if !defined(WIN32) && !defined(_WIN32) typedef unsigned short JSChar; #else typedef wchar_t JSChar; #endif ...
Quick wrap-up: We changed UChar in the Qt build on Windows to be also defined to wchar_t now. We removed the BUILDNG_QT #ifdef in API/JSStringRef.h. Simon
participants (6)
-
Alexey Proskuryakov
-
Artem Ananiev
-
Darin Adler
-
Lars Knoll
-
Maciej Stachowiak
-
Simon Hausmann