RESOLVED FIXED 53253
Move wince/mt19937ar.c to ThirdParty and make it a policy choice
https://bugs.webkit.org/show_bug.cgi?id=53253
Summary Move wince/mt19937ar.c to ThirdParty and make it a policy choice
Daniel Bates
Reported 2011-01-27 12:51:44 PST
We should make the decision to use the Merseene Twister pseudorandom number generator a policy choice and move the implementation of the Merseene Twister algorithm, in file wince/mt19937ar.c, to common location, say ThirdParty. Currently, the choice to use this pseudorandom number generator is dependent on whether we are building JavaScriptCore on Windows CE. Instead, we should make this a policy choice and enable this by default in the Windows CE port.
Attachments
Patch (15.62 KB, patch)
2011-01-27 13:35 PST, Daniel Bates
paroga: review-
Patch (17.88 KB, patch)
2011-01-27 14:43 PST, Daniel Bates
no flags
Patch (17.92 KB, patch)
2011-01-28 07:09 PST, Daniel Bates
no flags
Daniel Bates
Comment 1 2011-01-27 13:35:26 PST
Created attachment 80361 [details] Patch I thought to move the file wince/mt19937ar.c to Source/ThirdParty since it is a third party library. I am open to suggestions on where to place this file as well as the macro define to use. I am not very familiar with CMake and the Windows CE port so I am unsure of the correctness of the Cmake change. I used WebCore/CMakeLists.txt and JavaScriptCore/CMakeLists.txt as references.
Patrick R. Gansterer
Comment 2 2011-01-27 13:45:39 PST
Comment on attachment 80361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80361&action=review > Source/JavaScriptCore/CMakeListsWinCE.txt:3 > +LIST(APPEND JavaScriptCore_INCLUDE_DIRECTORIES > + "${CMAKE_SOURCE_DIR}/Source/ThirdParty" > +) ${CMAKE_SOURCE_DIR} is already <root>/Source, so it should read "${CMAKE_SOURCE_DIR}/ThirdParty" I'll try this change on my local build, because I'm not sure if it's the correct "*_INCLUDE_DIRECTORIES"
Daniel Bates
Comment 3 2011-01-27 14:00:06 PST
(In reply to comment #2) > (From update of attachment 80361 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80361&action=review > > > Source/JavaScriptCore/CMakeListsWinCE.txt:3 > > +LIST(APPEND JavaScriptCore_INCLUDE_DIRECTORIES > > + "${CMAKE_SOURCE_DIR}/Source/ThirdParty" > > +) > > ${CMAKE_SOURCE_DIR} is already <root>/Source, so it should read "${CMAKE_SOURCE_DIR}/ThirdParty" Will change. Based on your response then we should also change <http://trac.webkit.org/browser/trunk/Source/WebCore/CMakeLists.txt?rev=76792#L83>. > > I'll try this change on my local build, because I'm not sure if it's the correct "*_INCLUDE_DIRECTORIES" Actually, we should probably do this in change in file JavaScriptCore/wtf/CmakeListsWinCE.txt.
Early Warning System Bot
Comment 4 2011-01-27 14:05:35 PST
Patrick R. Gansterer
Comment 5 2011-01-27 14:12:01 PST
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 80361 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=80361&action=review > > > > > Source/JavaScriptCore/CMakeListsWinCE.txt:3 > > > +LIST(APPEND JavaScriptCore_INCLUDE_DIRECTORIES > > > + "${CMAKE_SOURCE_DIR}/Source/ThirdParty" > > > +) > > > > ${CMAKE_SOURCE_DIR} is already <root>/Source, so it should read "${CMAKE_SOURCE_DIR}/ThirdParty" > > Will change. FYI, IMHO the "correct" change: diff --git a/Source/JavaScriptCore/wtf/CMakeLists.txt b/Source/JavaScriptCore/wtf/CMakeLists.txt index b1931d7..9406f5d 100644 --- a/Source/JavaScriptCore/wtf/CMakeLists.txt +++ b/Source/JavaScriptCore/wtf/CMakeLists.txt @@ -136,6 +136,7 @@ INCLUDE_IF_EXISTS(${JAVASCRIPTCORE_DIR}/wtf/CMakeLists${PORT}.txt) LIST(APPEND WTF_INCLUDE_DIRECTORIES "${CMAKE_BINARY_DIR}" + "${CMAKE_SOURCE_DIR}/ThirdParty" ) WEBKIT_WRAP_SOURCELIST(${WTF_SOURCES}) OR diff --git a/Source/JavaScriptCore/wtf/CMakeListsWinCE.txt b/Source/JavaScriptCore/wtf/CMakeListsWinCE.txt index 9c558eb..19e9ceb 100644 --- a/Source/JavaScriptCore/wtf/CMakeListsWinCE.txt +++ b/Source/JavaScriptCore/wtf/CMakeListsWinCE.txt @@ -22,6 +22,10 @@ LIST(APPEND WTF_SOURCES ${3RDPARTY_DIR}/ce-compat/ce_unicode.cpp ) +LIST(APPEND WTF_INCLUDE_DIRECTORIES + "${CMAKE_SOURCE_DIR}/ThirdParty" +) + LIST(APPEND WTF_LIBRARIES mmtimer ) I like the first more, since you changed OS(WINCE) to USE(MERSENNE_TWISTER_19937). > Based on your response then we should also change > <http://trac.webkit.org/browser/trunk/Source/WebCore/CMakeLists.txt?rev=76792#L83>. Unfortunately CMake build system need some cleanup :-(. There are many "wrong" includes from the early "EFL only" days too.
Daniel Bates
Comment 6 2011-01-27 14:43:05 PST
Created attachment 80365 [details] Patch Updated patch based on Patrick Gansterer's suggestions. Appended Source/ThirdParty to the end of the list of include directories in JavaScriptCore/JavaScriptCore.pri in an attempt to fix the Qt Windows EWS bot.
Early Warning System Bot
Comment 7 2011-01-27 15:04:25 PST
Csaba Osztrogonác
Comment 8 2011-01-27 15:12:29 PST
Comment on attachment 80365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80365&action=review > Source/JavaScriptCore/JavaScriptCore.pri:26 > + $$PWD/../ThirdParty $$PWD/../ThirdParty \
Daniel Bates
Comment 9 2011-01-27 16:25:00 PST
(In reply to comment #8) > (From update of attachment 80365 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80365&action=review > > > Source/JavaScriptCore/JavaScriptCore.pri:26 > > + $$PWD/../ThirdParty > > $$PWD/../ThirdParty \ Thanks!
Daniel Bates
Comment 10 2011-01-28 07:09:27 PST
Eric Seidel (no email)
Comment 11 2011-01-28 12:14:04 PST
Comment on attachment 80448 [details] Patch OK.
Daniel Bates
Comment 12 2011-01-29 14:17:51 PST
Comment on attachment 80448 [details] Patch Clearing flags on attachment: 80448 Committed r77070: <http://trac.webkit.org/changeset/77070>
Daniel Bates
Comment 13 2011-01-29 14:17:58 PST
All reviewed patches have been landed. Closing bug.
Daniel Bates
Comment 14 2011-01-29 14:40:46 PST
(In reply to comment #5) > > Based on your response then we should also change > > <http://trac.webkit.org/browser/trunk/Source/WebCore/CMakeLists.txt?rev=76792#L83>. > Unfortunately CMake build system need some cleanup :-(. > There are many "wrong" includes from the early "EFL only" days too. Filed bug #53382 to remove references to ${CMAKE_SOURCE_DIR}/Source in CMake files.
Note You need to log in before you can comment on or make changes to this bug.