Summary: | [EFL] Replace alloca to C++ new placement | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | gyuyoung.kim, leandro, lucas.de.marchi, rakuco, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | Linux | ||||||||||||||||||
Attachments: |
|
Description
Grzegorz Czajkowski
2011-11-10 05:36:04 PST
Created attachment 114478 [details]
proposed patch
Comment on attachment 114478 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=114478&action=review > Source/WebCore/platform/efl/ScrollbarEfl.cpp:160 > + Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*> > + (alloca(sizeof(Edje_Message_Float_Set) + sizeof(float))); I'm not a big fan of alloca myself, as it is not very portable. Now that you're on this part of the code, it'd be good to replace it with new+delete or a smart pointer. > Source/WebKit/efl/ewk/ewk_cookies.cpp:104 > + } else > + CRITICAL("Could not allocate Ewk_Cookie."); Has this happened in your daily usage? I'm asking because every once in a while patches checking for memory allocations are sent and most of the time they are rejected for the same reason (there's not much point in checking for every allocation failure across WebKit/WebCore). > Source/WebKit/efl/ewk/ewk_window_features.cpp:126 > + if (!window_features->core) { > + CRITICAL("Could not allocate WebCore::WindowFeatures."); > + return 0; > + } Ditto. (In reply to comment #2) > (From update of attachment 114478 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114478&action=review > > > Source/WebCore/platform/efl/ScrollbarEfl.cpp:160 > > + Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*> > > + (alloca(sizeof(Edje_Message_Float_Set) + sizeof(float))); > > I'm not a big fan of alloca myself, as it is not very portable. Now that you're on this part of the code, it'd be good to replace it with new+delete or a smart pointer. Yes, you're right. As I know all new/delete and malloc/free calls will be replaced in one patch so it would be better to use new+delete here instead of alloca. > > > Source/WebKit/efl/ewk/ewk_cookies.cpp:104 > > + } else > > + CRITICAL("Could not allocate Ewk_Cookie."); > > Has this happened in your daily usage? I'm asking because every once in a while patches checking for memory allocations are sent and most of the time they are rejected for the same reason (there's not much point in checking for every allocation failure across WebKit/WebCore). No, it hasn't. But generally WebKit-EFL checks the most of allocation failure. And some patches have already been landed, for example https://bugs.webkit.org/show_bug.cgi?id=64932 https://bugs.webkit.org/show_bug.cgi?id=65853 Created attachment 117406 [details]
updated patch
Replaced alloca to new/delete in compliance with WebKit coding style.
Created attachment 117408 [details]
updated patch
In the previous patch only pointer was freed but whole table should be deleted.
Created attachment 117409 [details]
updated patch
Hi Kubo, I uploaded a new patch - alloca was changed to new/delete. Could you review this patch again? Comment on attachment 117409 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=117409&action=review I still think it is not needed to check for all these conditions. Speaking of other bug reports, https://bugs.webkit.org/show_bug.cgi?id=65408#c12 still makes a lot of sense (even though the patch ended up being committed). > Source/WebCore/platform/efl/ScrollbarEfl.cpp:162 > + Edje_Message_Float_Set* message = 0; > + char* mem = 0; > + if (mem = new char[sizeof(Edje_Message_Float_Set) + sizeof(double)]) > + message = new(mem) Edje_Message_Float_Set; This ended up looking quite ugly due to Edje's brain-dead way of manipulating this data structure. My suggestion is to just use Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>(malloc(sizeof(Edje_Message_Float_set) + sizeof(double))); if (!message) { // yadda yadda } and then free() message after sending it. If a reviewer prefers the use of placement new here, I suggest the following: char* mem = new char[sizeof(Edje_Message_Float_Set) * sizeof(double)]; Edje_Message_Float_Set* message = new(mem) Edje_Message_Float_Set; in this case, I don't even think it makes sense to check if the allocation fails, as new never returns NULL. > Source/WebKit/efl/ewk/ewk_cookies.cpp:104 > + } else > + CRITICAL("Could not allocate Ewk_Cookie."); I still wonder if this is really needed. Assuming the hypothetical case in which malloc fails in the middle of the loop, you nonetheless continue trying to allocate other Ewk_Cookies to add them to the list and then return. I don't think this helps the caller all that much. > Source/WebKit/efl/ewk/ewk_window_features.cpp:126 > + if (!window_features->core) { > + CRITICAL("Could not allocate WebCore::WindowFeatures."); > + return 0; > + } new never returns NULL. (In reply to comment #8) > (From update of attachment 117409 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117409&action=review > > I still think it is not needed to check for all these conditions. Speaking of other bug reports, https://bugs.webkit.org/show_bug.cgi?id=65408#c12 still makes a lot of sense (even though the patch ended up being committed). > > > Source/WebCore/platform/efl/ScrollbarEfl.cpp:162 > > + Edje_Message_Float_Set* message = 0; > > + char* mem = 0; > > + if (mem = new char[sizeof(Edje_Message_Float_Set) + sizeof(double)]) > > + message = new(mem) Edje_Message_Float_Set; > > This ended up looking quite ugly due to Edje's brain-dead way of manipulating this data structure. My suggestion is to just use > Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>(malloc(sizeof(Edje_Message_Float_set) + sizeof(double))); > if (!message) { > // yadda yadda > } > and then free() message after sending it. > > If a reviewer prefers the use of placement new here, I suggest the following: > char* mem = new char[sizeof(Edje_Message_Float_Set) * sizeof(double)]; > Edje_Message_Float_Set* message = new(mem) Edje_Message_Float_Set; > in this case, I don't even think it makes sense to check if the allocation fails, as new never returns NULL Ok, I will skip NULL checks for pointers initialized by new. > > > Source/WebKit/efl/ewk/ewk_cookies.cpp:104 > > + } else > > + CRITICAL("Could not allocate Ewk_Cookie."); > > I still wonder if this is really needed. Assuming the hypothetical case in which malloc fails in the middle of the loop, you nonetheless continue trying to allocate other Ewk_Cookies to add them to the list and then return. I don't think this helps the caller all that much. Do you prefer to stop loop in case of malloc failure and destroy (by free()) allocated memory? A similar solution in used in ewk_frame_source_get(). > > > Source/WebKit/efl/ewk/ewk_window_features.cpp:126 > > + if (!window_features->core) { > > + CRITICAL("Could not allocate WebCore::WindowFeatures."); > > + return 0; > > + } > > new never returns NULL. Ok, thanks. (In reply to comment #9) > (In reply to comment #8) > > > Source/WebKit/efl/ewk/ewk_cookies.cpp:104 > > > + } else > > > + CRITICAL("Could not allocate Ewk_Cookie."); > > > > I still wonder if this is really needed. Assuming the hypothetical case in which malloc fails in the middle of the loop, you nonetheless continue trying to allocate other Ewk_Cookies to add them to the list and then return. I don't think this helps the caller all that much. > > Do you prefer to stop loop in case of malloc failure and destroy (by free()) allocated memory? Well, I'm generally against the idea of checking all those allocations :) You will end up doing a lot more work for a hypothetical situation in which you are already pretty much screwed up. > A similar solution in used in ewk_frame_source_get(). Sorry, I couldn't find similar code in that function. Created attachment 120943 [details]
proposed patch
As a conclusion of above discussion NULL checking isn't needed in these cases.
I only replaced calling alloca to C++ new placement for three reasons:
1. alloca it is not very portable.
2. NULL checking isn't required as new never returns NULL,
3. in compliance with WebKit coding style.
Should be OK, albeit ugly :) Created attachment 121226 [details]
updated patch
Buffer is created on stack so we don't need to delete it.
Comment on attachment 121226 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=121226&action=review r=me > Source/WebCore/platform/efl/ScrollbarEfl.cpp:164 > - Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*> > - (alloca(sizeof(Edje_Message_Float_Set) + sizeof(float))); > + char buffer[sizeof(Edje_Message_Float_Set) + sizeof(double)]; > + Edje_Message_Float_Set* message = reinterpret_cast<Edje_Message_Float_Set*>(buffer); Please note in the ChangeLog why you are changing from float to double. Created attachment 121235 [details]
updated patch
Added reason of changing float to double to ChangeLog.
Patch for landing.
Comment on attachment 121235 [details] updated patch Clearing flags on attachment: 121235 Committed r104163: <http://trac.webkit.org/changeset/104163> All reviewed patches have been landed. Closing bug. |