Fixed compile time warning with respect to array and uninitialized variables.
Created attachment 158214 [details] patch
Comment on attachment 158214 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=158214&action=review > Source/WebCore/platform/efl/ScrollbarEfl.cpp:174 > - char buffer[sizeof(Edje_Message_Float_Set) + sizeof(double)]; > - Edje_Message_Float_Set* message = reinterpret_cast<Edje_Message_Float_Set*>(buffer); > + Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>(alloca(sizeof(Edje_Message_Float_Set) + sizeof(double))); Whenever you change weird constructs like this, you must check the logs and use svn blame first -- there is a reason for not using alloca() here.
Created attachment 158256 [details] patch Under current edje implementation, it seems we wouldn't avoid malloc usage even in cpp source code. :-(
(In reply to comment #3) > Created an attachment (id=158256) [details] > patch > > Under current edje implementation, it seems we wouldn't avoid malloc usage even in cpp source code. :-( Informal r- from my side because of two reason: 1) According to WebKit-Efl style we should avoid memory allocation with malloc and friends. new operator is preferred. 2) I don't see any benefits of allocating/freeing memory in this case. You can get more information here https://bugs.webkit.org/show_bug.cgi?id=72017.
(In reply to comment #4) > Informal r- from my side because of two reason: > > 1) According to WebKit-Efl style we should avoid memory allocation with malloc and friends. new operator is preferred. > > 2) I don't see any benefits of allocating/freeing memory in this case. > > You can get more information here https://bugs.webkit.org/show_bug.cgi?id=72017. Would you please suggest a way to remove build warning message under this circumstances?
Created attachment 158269 [details] patch How about this?
(In reply to comment #6) > Created an attachment (id=158269) [details] > patch > > How about this? Thanks for the patch and newly introduced solution how to avoid problems with a little problematic Edje_Message_Float_Set struct. In the origin code 'buffer' is created on stack, so we don't need to take of freeing memory and possibles memory leaks but compiler complains on 'message' pointer after reinterpret_cast that it is not initialized. Your patch creates 'buffer' on heap and it's deleted when function returns (delete [] buffer). Additionally you have to twice cast, char * -> void and then void* -> Edje_Message_Float_Set*. Is it really worth to resolve this compilation warning? Kubo what do you think?
Comment on attachment 158269 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=158269&action=review > Source/WebCore/ChangeLog:17 > + * platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp: > + (WebCore::HarfBuzzShaper::selectionRect): fix 'toX and fromX may be used uninitialized in this function' warning. Note that this isn't related to the bug title anymore :-)
(In reply to comment #7) > Is it really worth to resolve this compilation warning? > Kubo what do you think? I tried the following snippet here: char buffer[sizeof(Edje_Message_Float_Set) + sizeof(double)]; Edje_Message_Float_Set *message = reinterpret_cast<Edje_Message_Float_Set*>(buffer); and neither gcc 4.7.1 nor clang 3.1 with -Wall and -Wextra have raised any warning such as the one Kangil mentions in the ChangeLog. Kangil, what compiler are you using and what flags are being passed to it when building these files?
(In reply to comment #9) > I tried the following snippet here: > > char buffer[sizeof(Edje_Message_Float_Set) + sizeof(double)]; > Edje_Message_Float_Set *message = reinterpret_cast<Edje_Message_Float_Set*>(buffer); > > and neither gcc 4.7.1 nor clang 3.1 with -Wall and -Wextra have raised any warning such as the one Kangil mentions in the ChangeLog. > > Kangil, what compiler are you using and what flags are being passed to it when building these files? Here it is. :-) { gcc (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1 /home/hankangil/dev/WebKit/webkit-gtk/WebKit/Source/WebCore/platform/efl/ScrollbarEfl.cpp:184:23: warning: array subscript is above array bounds [-Warray-bounds] /home/hankangil/dev/WebKit/webkit-gtk/WebKit/Source/WebCore/platform/efl/ScrollbarEfl.cpp:186:23: warning: array subscript is above array bounds [-Warray-bounds] } (In reply to comment #8) > Note that this isn't related to the bug title anymore :-) I will select better title after figuring out the direction how I could resolve build warning with respect to strange edje message stuff. ;-)
(In reply to comment #10) > > and neither gcc 4.7.1 nor clang 3.1 with -Wall and -Wextra have raised any warning such as the one Kangil mentions in the ChangeLog. > > > > Kangil, what compiler are you using and what flags are being passed to it when building these files? > > Here it is. :-) > { > gcc (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1 Right. I've tried the following snippet with gcc 4.6.3 and it didn't produce any warnings either: #include <Edje.h> int main() { char buffer[sizeof(Edje_Message_Float_Set) + sizeof(double)]; Edje_Message_Float_Set* message = reinterpret_cast<Edje_Message_Float_Set*>(buffer); message->count = 2; message->val[0] = 0.0; message->val[1] = 1.0; return 0; } Does this code make your compiler produce any kind of warning?
(In reply to comment #11) > Does this code make your compiler produce any kind of warning? I used 'g++ (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1' and found *NO* error on your code snippet. This is very interesting!!
Here is interesting result I've found. It depends on gcc optimization flag. -O0(no optimization) : no warning message. -O1 : no warning message. -O2 : test.cpp:16:19: warning: array subscript is above array bounds [-Warray-bounds] -O3(actually used in efl build) : test.cpp:16:19: warning: array subscript is above array bounds [-Warray-bounds] Given circumstances, my two cents on this, I know edje has implemented unusually, using *new* keyword would be unavoidable. rakuco, grzegorz: how do you think about this?
OK, I've managed to reproduce that warning with my code snippet by calling edje_object_message_send() in it (clang still doesn't produce any warning). There are two possible solutions here: 1) Use plain old malloc() here specifying the amount of bytes we want to allocate and avoiding all those casts. 2) Refining Kangil's proposed patch. Solution 2) is interesting because the static_cast<void*>() is not necessary, so we can remove some bloat. We could also pass the allocated memory to an OwnPtr, thus avoiding the need to free the memory manually. Kangil, can you produce a new patch following those suggestions (please also refrain from changing variable names to keep it as short as possible)?
(In reply to comment #14) > OK, I've managed to reproduce that warning with my code snippet by calling edje_object_message_send() in it (clang still doesn't produce any warning). > > There are two possible solutions here: > > 1) Use plain old malloc() here specifying the amount of bytes we want to allocate and avoiding all those casts. > > 2) Refining Kangil's proposed patch. > > Solution 2) is interesting because the static_cast<void*>() is not necessary, so we can remove some bloat. We could also pass the allocated memory to an OwnPtr, thus avoiding the need to free the memory manually. > > Kangil, can you produce a new patch following those suggestions (please also refrain from changing variable names to keep it as short as possible)? Sure, I will get back with an updated one. ;-) Thanks!
(In reply to comment #14) > OK, I've managed to reproduce that warning with my code snippet by calling edje_object_message_send() in it (clang still doesn't produce any warning). > > There are two possible solutions here: > > 1) Use plain old malloc() here specifying the amount of bytes we want to allocate and avoiding all those casts. > How about using new placement instead? Does it produce warning too? I couldn't verify it as I am facing problem to access svn.enlightenment.org and git.webkit.org :/ sorry char* buffer = new char[sizeof(Edje_Message_Float_Set) + sizeof(double)]) Edje_Message_Float_Set* message = new(buffer) Edje_Message_Float_Set; This plus smart pointer may be another solution for us.
(In reply to comment #16) > How about using new placement instead? Does it produce warning too? I don't think you can use placement new here, as Edje_Message_Float_Set is a C type that does not have a constructor or a placement new overload.
(In reply to comment #17) > (In reply to comment #16) > > How about using new placement instead? Does it produce warning too? > > I don't think you can use placement new here, as Edje_Message_Float_Set is a C type that does not have a constructor or a placement new overload. Hmm, I've been trying to use this one at https://bugs.webkit.org/show_bug.cgi?id=72017 (see 5th patch). As I remember it has worked.
(In reply to comment #18) > Hmm, I've been trying to use this one at https://bugs.webkit.org/show_bug.cgi?id=72017 (see 5th patch). As I remember it has worked. Don't worry. I will test your code snippet and get back with result. :-)
grzegorz's code snippet works. :-) rakuco, grzegorz: we can pick better one. which one would you prefer?
(In reply to comment #20) > grzegorz's code snippet works. :-) Hmm, it was failing here because <new> had to be included in that snippet I was using. Go for the placement new version, as it is cleaner and conveys the intents better than the casts. Just for safety, please #include <new> as well to make sure the global placement new overload is available.
(In reply to comment #21) > Hmm, it was failing here because <new> had to be included in that snippet I was using. > > Go for the placement new version, as it is cleaner and conveys the intents better than the casts. Just for safety, please #include <new> as well to make sure the global placement new overload is available. new version has one drawback. it seems we couldn't use OwnPtr for this case. Would you prefer explicit *delete* usage?
Created attachment 159017 [details] patch
Comment on attachment 159017 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=159017&action=review > Source/WebCore/ChangeLog:3 > + [EFL] Remove alloca usage and fix compile warning in WebCore The original title about removing warnings from WebCore makes more sense here, as removing alloca is part of this process. > Source/WebCore/ChangeLog:9 > + To keep portable source code, alloca has been discouraged. > + Therefore, this patch removed alloca usage from EFL port. I would mention here that we're unifying the approaches for the creation of Edje_Message_Float_Set messages, and getting rid of alloca is part of it, since it is not portable. It'd also be good to mention the compiler warnings you were getting. > Source/WebCore/platform/efl/RenderThemeEfl.cpp:360 > + edje_object_message_send(entry->o, EDJE_MESSAGE_FLOAT_SET, 0, msg); See the comment in line 327; you do not need to send this message unconditionally. > Source/WebCore/platform/efl/RenderThemeEfl.cpp:450 > + EINA_LOG_ERR("Could not set file: %s", errmsg); Not really related to this patch. > Source/WebCore/platform/efl/ScrollbarEfl.cpp:175 > + Edje_Message_Float_Set* msg = new(buffer.get()) Edje_Message_Float_Set; The renaming of this variable makes the patch needlessly bigger.
(In reply to comment #24) > > Source/WebCore/platform/efl/RenderThemeEfl.cpp:360 > > + edje_object_message_send(entry->o, EDJE_MESSAGE_FLOAT_SET, 0, msg); > > See the comment in line 327; you do not need to send this message unconditionally. > I couldn't see your comment in line 327. What was that about?
(In reply to comment #25) > I couldn't see your comment in line 327. What was that about? http://trac.webkit.org/browser/trunk/Source/WebCore/platform/efl/RenderThemeEfl.cpp#L327
Created attachment 159060 [details] patch
Comment on attachment 159060 [details] patch Looks great, thanks!
(In reply to comment #28) > (From update of attachment 159060 [details]) > Looks great, thanks! Thanks for great review, too!
Informal r+ from my side too.
(In reply to comment #30) > Informal r+ from my side too. Thanks grzegorz. Your suggestion(using *new*) has helped me a lot. :-)
Comment on attachment 159060 [details] patch LGTM
Comment on attachment 159060 [details] patch Clearing flags on attachment: 159060 Committed r125883: <http://trac.webkit.org/changeset/125883>
All reviewed patches have been landed. Closing bug.