Summary: | OpcodeStats doesn't build on platforms which don't have mergesort() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Minor | CC: | ggaren, mjs, oliver | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Csaba Osztrogonác
2008-08-06 04:35:24 PDT
Created attachment 22676 [details]
proposed patch
Comment on attachment 22676 [details]
proposed patch
Nah. Better would be to either #define mergesort qsort
or to build some sort of abstraction for sorting "platformMergeSort()" and only have the #ifdef in one place.
I'm surprised this hasn't been done already somewhere else? You might try searching the sources for HAVE(MERGE_SORT)
(In reply to comment #2) > I'm surprised this hasn't been done already somewhere else? You might try > searching the sources for HAVE(MERGE_SORT) I searched for HAVE(MERGESORT), and found it kjs/JSArray.cpp and VM/SamplingTools.cpp. HAVE_MERGESORT macro defined in wtf/Platform.h. I think this is a commonly used macro. Your advice about platformMergeSort() can be a nice solution. What do you think, if I would refactor my patch and extend it the above mentioned files? Created attachment 22694 [details] new patch based on Comment #2 I refactored using mergesort based on your advices. What about this preliminary patch? Created attachment 22933 [details]
This is my new proposed patch.
Comment on attachment 22933 [details]
This is my new proposed patch.
This should not be defined in Platform.h -- that goes beyond the scope of what Platform.h should contain; just configuration macros. It belongs in a header like MathExtras.h -- perhaps StdLibExtras.h.
This should be an inline function, not a macro. If it was going to be a macro it should be a macro with arguments, not an unqualified macro.
The name platformSort doesn't make sense to me. If we need mergesort specifically, then it needs to be mergesort. If it can be either qsort or mergesort, then we can come up with a name that describes the reason we want mergesort if we have it -- what is that reason? Why is mergesort better than qsort for these purposes? I don't think "platform" enters into it, despite Eric's earlier suggestion.
For example, in JSArray.cpp it's pretty clear what we want is a stable sort. In those cases, using qsort is actually incorrect, so I don't think we should try to make it more elegant -- the lack of a stable sort there is a problem needs to be fixed.
Is stable sorting the issue in Opcode.cpp too? If we don't have any special requirements, then it we can just change Opcode.cpp to use qsort instead of doing this whole thing.
My apologies -- I'm not trying to make this more complicated than it is. You're right, in JSArray.cpp we need stable sort. I think we don't need stable sort in Opcode.cpp. OpcodeStats is only generates statistics for developers, it doesn't modify the funcionality of jsc release version. Created attachment 22974 [details]
proposed patch
Comment on attachment 22974 [details]
proposed patch
r=me
Landed in r36048. |