| Summary: | [EFL] Use C++ type cast instead of C style type cast | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||||||
| Component: | WebKit EFL | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | gyuyoung.kim, leandro, lucas.de.marchi, mrobinson, rakuco, webkit.review.bot | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 68209 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Gyuyoung Kim
2011-09-18 00:37:02 PDT
Created attachment 108354 [details]
Patch
Comment on attachment 108354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108354&action=review I was going to comment on all occurrences which changed "Foo* p = ..." to "Foo *p = ..." but gave up about halfway into the patch. > Source/WebKit/efl/ewk/ewk_auth_soup.cpp:82 > + Ewk_Auth_Data *auth_data = static_cast<Ewk_Auth_Data*>(data); Wrong '*' position. > Source/WebKit/efl/ewk/ewk_contextmenu.cpp:106 > + Ewk_Context_Menu_Item *item = static_cast<Ewk_Context_Menu_Item*>(malloc(sizeof(*item))); Ditto. > Source/WebKit/efl/ewk/ewk_frame.cpp:689 > + Ewk_Hit_Test *hit_test = static_cast<Ewk_Hit_Test*>(calloc(1, sizeof(Ewk_Hit_Test))); Ditto. > Source/WebKit/efl/ewk/ewk_view.cpp:210 > + Ewk_View_Smart_Data *ptr = static_cast<Ewk_View_Smart_Data*>(evas_object_smart_data_get(o)) The placement of the first '*' could be fixed here. > Source/WebKit/efl/ewk/ewk_view.cpp:473 > + Ewk_View_Smart_Data *sd = static_cast<Ewk_View_Smart_Data*>(data); Wrong position for the first '*'. > Source/WebKit/efl/ewk/ewk_view.cpp:481 > + Ewk_View_Smart_Data *sd = static_cast<Ewk_View_Smart_Data*>(data); Ditto. > Source/WebKit/efl/ewk/ewk_view.cpp:727 > + const Ewk_View_Smart_Class* api = (Ewk_View_Smart_Class*)(sc); No C++ cast here. (In reply to comment #2) > (From update of attachment 108354 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108354&action=review > > I was going to comment on all occurrences which changed "Foo* p = ..." to "Foo *p = ..." but gave up about halfway into the patch. Gyuyoung, you might want to apply the uncrustify config on top of this patch and check if it changed back the position of '*'. Created attachment 108441 [details]
Patch
Sorry, I missed to fix '*' operator in my local changes despite running git pull. I run uncrustify.cfg in my files. Fortunately, I found remained wrong pointer operator placement. I fix them as well. >> Source/WebKit/efl/ewk/ewk_view.cpp:727 >> + const Ewk_View_Smart_Class* api = (Ewk_View_Smart_Class*)(sc); > No C++ cast here. I fix this line using reinterpret_cast because of different pointer casting. LGTM. Comment on attachment 108441 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108441&action=review > Source/WebKit/efl/ChangeLog:60 > + * ewk/ewk_auth_soup.cpp: > + (_Ewk_Auth_Data::ewk_auth_soup_credentials_set): > + (_Ewk_Auth_Data::session_authenticate): > + * ewk/ewk_contextmenu.cpp: > + (ewk_context_menu_item_new): > + * ewk/ewk_frame.cpp: > + (_ewk_frame_smart_add): > + (ewk_frame_children_iterator_new): > + (ewk_frame_hit_test_new): > + * ewk/ewk_history.cpp: > + (_ewk_history_item_new): > + (ewk_history_item_title_get): > + (ewk_history_item_title_alternate_get): > + (ewk_history_item_uri_get): > + (ewk_history_item_uri_original_get): > + (ewk_history_new): > + * ewk/ewk_view.cpp: > + (_ewk_view_repaints_resize): > + (_ewk_view_scrolls_resize): > + (_ewk_view_on_focus_in): > + (_ewk_view_on_focus_out): > + (_ewk_view_on_mouse_wheel): > + (_ewk_view_on_mouse_down): > + (_ewk_view_on_mouse_up): > + (_ewk_view_on_mouse_move): > + (_ewk_view_on_key_down): > + (_ewk_view_on_key_up): > + (_ewk_view_priv_new): > + (_ewk_view_smart_add): > + (_ewk_view_smart_zoom_set): > + (_ewk_view_zoom_animator_cb): > + (_ewk_view_viewport_attributes_compute): > + (ewk_view_zoom_set): > + (ewk_view_zoom_weak_set): > + (ewk_view_zoom_animated_set): > + (ewk_view_paint_context_new): > + (ewk_view_popup_new): > + (ewk_view_popup_destroy): > + * ewk/ewk_view_single.cpp: > + (_ewk_view_single_smart_add): > + (_ewk_view_single_smart_resize): > + (_ewk_view_single_scroll_process_single): > + (_ewk_view_single_smart_repaints_process): > + * ewk/ewk_view_tiled.cpp: > + (_ewk_view_tiled_render_cb): > + (_ewk_view_tiled_updates_process_pre): > + (_ewk_view_tiled_contents_size_changed_cb): > + (_ewk_view_tiled_smart_add): As mentioned by Antonio Gomes in the other giant patch, please remove this from ChangeLog since it's not adding any useful information. > Source/WebKit/efl/ewk/ewk_view.cpp:1564 > - WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom); > + WRN("zoom level is < %f : %f", static_cast<double>(priv->settings.zoom_range.min_scale), static_cast<double>(zoom)); I'm surprised to see we are casting a float to a double. This is a nice time to remove that. > Source/WebKit/efl/ewk/ewk_view.cpp:1568 > - WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom); > + WRN("zoom level is > %f : %f", static_cast<double>(priv->settings.zoom_range.max_scale), static_cast<double>(zoom)); ditto. > Source/WebKit/efl/ewk/ewk_view.cpp:1608 > - WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom); > + WRN("zoom level is < %f : %f", static_cast<double>(priv->settings.zoom_range.min_scale), static_cast<double>(zoom)); ditto. > Source/WebKit/efl/ewk/ewk_view.cpp:1612 > - WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom); > + WRN("zoom level is > %f : %f", static_cast<double>(priv->settings.zoom_range.max_scale), static_cast<double>(zoom)); ditto. > Source/WebKit/efl/ewk/ewk_view.cpp:1664 > - WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom); > + WRN("zoom level is < %f : %f", static_cast<double>(priv->settings.zoom_range.min_scale), static_cast<double>(zoom)); ditto. > Source/WebKit/efl/ewk/ewk_view.cpp:1668 > - WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom); > + WRN("zoom level is > %f : %f", static_cast<double>(priv->settings.zoom_range.max_scale), static_cast<double>(zoom)); ditto. Created attachment 108604 [details]
Patch
This is nice time to fix wrong casting. I fixed them. And, meaningless comments are removed from ChangeLog as well. Comment on attachment 108604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108604&action=review Please, check the unneeded casts. Especially the float -> double conversions. > Source/WebKit/efl/ewk/ewk_view.cpp:929 > - px = (double)(x + cx) / (w + sd->view.w); > + px = static_cast<double>((x + cx) / (w + sd->view.w)); Unneeded cast. > Source/WebKit/efl/ewk/ewk_view.cpp:934 > - py = (double)(y + cy) / (h + sd->view.h); > + py = static_cast<double>((y + cy) / (h + sd->view.h)); ditto. > Source/WebKit/efl/ewk/ewk_view.cpp:1052 > - int available_width = (int) priv->page->chrome()->client()->pageRect().width(); > - int available_height = (int) priv->page->chrome()->client()->pageRect().height(); > + int available_width = static_cast<int>(priv->page->chrome()->client()->pageRect().width()); > + int available_height = static_cast<int>(priv->page->chrome()->client()->pageRect().height()); /me wondering why we are going priv->page->chrome()->client() route when we have the ewk_view available. It would be just a matter of calling ewk_view_page_rect_get(). About the casts, we are probably better off using enclosingIntRect() instead. > Source/WebKit/efl/ewk/ewk_view.cpp:1055 > - int device_width = (int) priv->page->chrome()->client()->windowRect().width(); > - int device_height = (int) priv->page->chrome()->client()->windowRect().height(); > + int device_width = static_cast<int>(priv->page->chrome()->client()->windowRect().width()); > + int device_height = static_cast<int>(priv->page->chrome()->client()->windowRect().height()); ditto. > Source/WebKit/efl/ewk/ewk_view.cpp:1564 > - WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom); > + WRN("zoom level is < %f : %f", static_cast<float>(priv->settings.zoom_range.min_scale), static_cast<float>(zoom)); Unneeded casts. > Source/WebKit/efl/ewk/ewk_view.cpp:1568 > - WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom); > + WRN("zoom level is > %f : %f", static_cast<float>(priv->settings.zoom_range.max_scale), static_cast<float>(zoom)); Unneeded casts. > Source/WebKit/efl/ewk/ewk_view.cpp:1608 > - WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom); > + WRN("zoom level is < %f : %f", static_cast<float>(priv->settings.zoom_range.min_scale), static_cast<float>(zoom)); Unneeded casts. > Source/WebKit/efl/ewk/ewk_view.cpp:1612 > - WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom); > + WRN("zoom level is > %f : %f", static_cast<float>(priv->settings.zoom_range.max_scale), static_cast<float>(zoom)); Unneeded casts. > Source/WebKit/efl/ewk/ewk_view.cpp:1664 > - WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom); > + WRN("zoom level is < %f : %f", static_cast<float>(priv->settings.zoom_range.min_scale), static_cast<float>(zoom)); Unneeded casts. > Source/WebKit/efl/ewk/ewk_view.cpp:1668 > - WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom); > + WRN("zoom level is > %f : %f", static_cast<float>(priv->settings.zoom_range.max_scale), static_cast<float>(zoom)); Unneeded casts. >> /me wondering why we are going priv->page->chrome()->client() route when we have the ewk_view available. It would be just a matter of calling ewk_view_page_rect_get(). >> About the casts, we are probably better off using enclosingIntRect() instead. I think that is modified by new bug. So, I think it is better to use static_cast<..> for this bug as below, 1051 int available_width = static_cast<int>(priv->page->chrome()->client()->pageRect().width()); 1052 int available_height = static_cast<int>(priv->page->chrome()->client()->pageRect().height()); 1053 1054 int device_width = static_cast<int>(priv->page->chrome()->client()->windowRect().width()); 1055 int device_height = static_cast<int>(priv->page->chrome()->client()->windowRect().height()); Do you think that is fixed first ? (In reply to comment #11) > >> /me wondering why we are going priv->page->chrome()->client() route when we have the ewk_view available. It would be just a matter of calling ewk_view_page_rect_get(). > > >> About the casts, we are probably better off using enclosingIntRect() instead. > > I think that is modified by new bug. So, I think it is better to use static_cast<..> for this bug as below, > > 1051 int available_width = static_cast<int>(priv->page->chrome()->client()->pageRect().width()); > 1052 int available_height = static_cast<int>(priv->page->chrome()->client()->pageRect().height()); > 1053 > 1054 int device_width = static_cast<int>(priv->page->chrome()->client()->windowRect().width()); > 1055 int device_height = static_cast<int>(priv->page->chrome()->client()->windowRect().height()); > ok. Created attachment 108962 [details]
Patch
Comment on attachment 108962 [details] Patch Attachment 108962 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9881573 Created attachment 108967 [details]
Patch
Hello Lucas, My though is a little changed. It seems it is better to remove static_cast<...> of _ewk_view_viewport_attributes_compute() in this bug. WebCore::IntRect available_rect = enclosingIntRect(priv->page->chrome()->client()->pageRect()); int available_width = available_rect.width(); int available_height = available_rect.height(); WebCore::IntRect device_rect = enclosingIntRect(priv->page->chrome()->client()->windowRect()); int device_width = device_rect.width(); int device_height = device_rect.height(); Of course, we can file new bug to decide which functions are used instead of pageRect(), windowRect(). Comment on attachment 108967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108967&action=review Otherwise looks good now. > Source/WebKit/efl/ewk/ewk_view.cpp:1053 > - int available_width = (int) priv->page->chrome()->client()->pageRect().width(); > - int available_height = (int) priv->page->chrome()->client()->pageRect().height(); > + WebCore::IntRect available_rect = enclosingIntRect(priv->page->chrome()->client()->pageRect()); > + int available_width = available_rect.width(); > + int available_height = available_rect.height(); WebCore::IntRect availableRect = enclosingIntRect(priv->page->chrome()->client()->pageRect()); and you don't need available_{width,height}. Just use .width()/.height() below. > Source/WebKit/efl/ewk/ewk_view.cpp:1057 > - int device_width = (int) priv->page->chrome()->client()->windowRect().width(); > - int device_height = (int) priv->page->chrome()->client()->windowRect().height(); > + WebCore::IntRect device_rect = enclosingIntRect(priv->page->chrome()->client()->windowRect()); > + int device_width = device_rect.width(); > + int device_height = device_rect.height(); Same here. deviceRect would be more WebKit-style aligned. (In reply to comment #17) > (From update of attachment 108967 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108967&action=review > > Otherwise looks good now. > > > Source/WebKit/efl/ewk/ewk_view.cpp:1053 > > - int available_width = (int) priv->page->chrome()->client()->pageRect().width(); > > - int available_height = (int) priv->page->chrome()->client()->pageRect().height(); > > + WebCore::IntRect available_rect = enclosingIntRect(priv->page->chrome()->client()->pageRect()); > > + int available_width = available_rect.width(); > > + int available_height = available_rect.height(); > > WebCore::IntRect availableRect = enclosingIntRect(priv->page->chrome()->client()->pageRect()); > > and you don't need available_{width,height}. Just use .width()/.height() below. > > > Source/WebKit/efl/ewk/ewk_view.cpp:1057 > > - int device_width = (int) priv->page->chrome()->client()->windowRect().width(); > > - int device_height = (int) priv->page->chrome()->client()->windowRect().height(); > > + WebCore::IntRect device_rect = enclosingIntRect(priv->page->chrome()->client()->windowRect()); > > + int device_width = device_rect.width(); > > + int device_height = device_rect.height(); > > Same here. deviceRect would be more WebKit-style aligned. efl port is using mixed efl-style and webkit-style for local variable. I think we should make a rule for local variable. This is an new item for Bug 68321. Do you want to remove "_" in available_rect and device_rect in this bug ? Comment on attachment 108967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108967&action=review >>> Source/WebKit/efl/ewk/ewk_view.cpp:1053 >>> + int available_height = available_rect.height(); >> >> WebCore::IntRect availableRect = enclosingIntRect(priv->page->chrome()->client()->pageRect()); >> >> and you don't need available_{width,height}. Just use .width()/.height() below. > > efl port is using mixed efl-style and webkit-style for local variable. I think we should make a rule for local variable. This is an new item for Bug 68321. Do you want to remove "_" in available_rect and device_rect in this bug ? I don't care too much about the name now (since it can be made in another bug). Just send another patch removing the useless available_width, available_height, device_width and device_height variables. Then you have my informal r+. Created attachment 109168 [details]
Patch
I remove useless local variables in _ewk_view_viewport_attributes_compute() according to your comment. Comment on attachment 109168 [details]
Patch
Looks good. Informal r+
Comment on attachment 109168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109168&action=review Nice cleanup, but I have some concerns. > Source/WebKit/efl/ewk/ewk_view.cpp:930 > - px = (double)(x + cx) / (w + sd->view.w); > + px = (x + cx) / (w + sd->view.w); > else Are you sure a cast isn't required here to force floating point arithmetic? > Source/WebKit/efl/ewk/ewk_view.cpp:934 > - py = (double)(y + cy) / (h + sd->view.h); > + py = (y + cy) / (h + sd->view.h); Ditto. > Source/WebKit/efl/ewk/ewk_view.cpp:1055 > + WebCore::IntSize available_size = WebCore::IntSize(available_rect.width(), available_rect.height()); > + WebCore::ViewportAttributes attributes = WebCore::computeViewportAttributes(priv->viewport_arguments, desktop_width, device_rect.width(), device_rect.height(), device_dpi, available_size); You should just do available_rect.size() > Source/WebKit/efl/ewk/ewk_view.cpp:1565 > - WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom); > + WRN("zoom level is < %f : %f", priv->settings.zoom_range.min_scale, zoom); > return EINA_FALSE; > } > if (zoom > priv->settings.zoom_range.max_scale) { > - WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom); > + WRN("zoom level is > %f : %f", priv->settings.zoom_range.max_scale, zoom); Are these values not floats? If so why not fix the actual format specifiers instead of just removing the cast? (In reply to comment #23) > (From update of attachment 109168 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109168&action=review > > Nice cleanup, but I have some concerns. > > > Source/WebKit/efl/ewk/ewk_view.cpp:930 > > - px = (double)(x + cx) / (w + sd->view.w); > > + px = (x + cx) / (w + sd->view.w); > > else > > Are you sure a cast isn't required here to force floating point arithmetic? This was my bad. I didn't pay attention w and sd->view.w are of type Evas_Coord and not double. > > Source/WebKit/efl/ewk/ewk_view.cpp:1565 > > - WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom); > > + WRN("zoom level is < %f : %f", priv->settings.zoom_range.min_scale, zoom); > > return EINA_FALSE; > > } > > if (zoom > priv->settings.zoom_range.max_scale) { > > - WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom); > > + WRN("zoom level is > %f : %f", priv->settings.zoom_range.max_scale, zoom); > > Are these values not floats? If so why not fix the actual format specifiers instead of just removing the cast? These values are floats. This is the reason the cast is not needed. (In reply to comment #23) > (From update of attachment 109168 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109168&action=review > > Nice cleanup, but I have some concerns. > > > Source/WebKit/efl/ewk/ewk_view.cpp:930 > > - px = (double)(x + cx) / (w + sd->view.w); > > + px = (x + cx) / (w + sd->view.w); > > else > > Are you sure a cast isn't required here to force floating point arithmetic? I think it is better to use a cast explicitly. Frankly I am not sure. > > Source/WebKit/efl/ewk/ewk_view.cpp:934 > > - py = (double)(y + cy) / (h + sd->view.h); > > + py = (y + cy) / (h + sd->view.h); > > Ditto. > > > Source/WebKit/efl/ewk/ewk_view.cpp:1055 > > + WebCore::IntSize available_size = WebCore::IntSize(available_rect.width(), available_rect.height()); > > + WebCore::ViewportAttributes attributes = WebCore::computeViewportAttributes(priv->viewport_arguments, desktop_width, device_rect.width(), device_rect.height(), device_dpi, available_size); > > You should just do available_rect.size() I missed to use .size(). I will fix it. > > Source/WebKit/efl/ewk/ewk_view.cpp:1565 > > - WRN("zoom level is < %f : %f", (double)priv->settings.zoom_range.min_scale, (double)zoom); > > + WRN("zoom level is < %f : %f", priv->settings.zoom_range.min_scale, zoom); > > return EINA_FALSE; > > } > > if (zoom > priv->settings.zoom_range.max_scale) { > > - WRN("zoom level is > %f : %f", (double)priv->settings.zoom_range.max_scale, (double)zoom); > > + WRN("zoom level is > %f : %f", priv->settings.zoom_range.max_scale, zoom); > > Are these values not floats? If so why not fix the actual format specifiers instead of just removing the cast? max_scale and zoom variables are float type. So, IMHO, it is better to use "%f" format specifier without casting. Created attachment 109226 [details]
Patch
(In reply to comment #25) > (In reply to comment #23) > > (From update of attachment 109168 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=109168&action=review > > > > Nice cleanup, but I have some concerns. > > > > > Source/WebKit/efl/ewk/ewk_view.cpp:930 > > > - px = (double)(x + cx) / (w + sd->view.w); > > > + px = (x + cx) / (w + sd->view.w); > > > else > > > > Are you sure a cast isn't required here to force floating point arithmetic? > > I think it is better to use a cast explicitly. Frankly I am not sure. If Evas_Coord is an integer type then I believe it's necessary. Comment on attachment 109226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109226&action=review > Source/WebKit/efl/ewk/ewk_view.cpp:929 > + px = static_cast<double>((x + cx) / (w + sd->view.w)); Casting the entire expression here is semantically identical to the implicit cast. You need to at least cast the first operand of the division. Created attachment 109229 [details]
Patch
Comment on attachment 109229 [details]
Patch
Nice cleanup!
Comment on attachment 109229 [details] Patch Clearing flags on attachment: 109229 Committed r96382: <http://trac.webkit.org/changeset/96382> All reviewed patches have been landed. Closing bug. |