Bug 143750 - Use UNUSED_PARAM instead of the void casting to suppress unused parameter warnings
Summary: Use UNUSED_PARAM instead of the void casting to suppress unused parameter war...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-15 01:44 PDT by Sungmann Cho
Modified: 2015-04-16 18:26 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.41 KB, patch)
2015-04-15 01:45 PDT, Sungmann Cho
no flags Details | Formatted Diff | Diff
Patch (1.45 KB, patch)
2015-04-15 01:56 PDT, Sungmann Cho
no flags Details | Formatted Diff | Diff
Patch (1.52 KB, patch)
2015-04-15 08:44 PDT, Sungmann Cho
no flags Details | Formatted Diff | Diff
Patch (1.63 KB, patch)
2015-04-15 09:34 PDT, Sungmann Cho
no flags Details | Formatted Diff | Diff
Patch (1.97 KB, patch)
2015-04-16 17:30 PDT, Sungmann Cho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sungmann Cho 2015-04-15 01:44:47 PDT
Use UNUSED_PARAM instead of the void casting.
Comment 1 Sungmann Cho 2015-04-15 01:45:47 PDT
Created attachment 250780 [details]
Patch
Comment 2 Sungmann Cho 2015-04-15 01:56:57 PDT
Created attachment 250781 [details]
Patch
Comment 3 Csaba Osztrogonác 2015-04-15 08:04:16 PDT
Comment on attachment 250781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250781&action=review

> Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:65
> +    UNUSED_PARAM(page);

Using UNUSED_PARAM(page) unconditionally is incorrect, it should be in 
the #else case of ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
Comment 4 Sungmann Cho 2015-04-15 08:44:00 PDT
Created attachment 250791 [details]
Patch
Comment 5 Sungmann Cho 2015-04-15 08:54:08 PDT
Thank you for pointing that out. I updated the patch.
Please take a look. Thanks!
Comment 6 Gyuyoung Kim 2015-04-15 08:59:10 PDT
Comment on attachment 250791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250791&action=review

> Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:65
> +#if !ENABLE(NOTIFICATIONS) && !ENABLE(LEGACY_NOTIFICATIONS)

I don't like to use unnecessary macro. Probably it looks Ossy wants to use a thing similar to below style. 

NotificationPermissionRequestManager::NotificationPermissionRequestManager(WebPage* page)
{
#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
    m_page = page;
#else
    UNUSED_PARAM(page);
#endif
}
Comment 7 Csaba Osztrogonác 2015-04-15 09:00:11 PDT
Comment on attachment 250791 [details]
Patch

Ouch, it's so ugly. :-/ Unfortunately we can't put UNUSED_PARAM to the same place as init list.

I think this one would be the least ugly solution

NotificationPermissionRequestManager::NotificationPermissionRequestManager(WebPage* page)
{
#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
    m_page = page
#else
    UNUSED_PARAM(page);
#endif
}
Comment 8 Csaba Osztrogonác 2015-04-15 09:01:38 PDT
(In reply to comment #6)
> Comment on attachment 250791 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250791&action=review
> 
> > Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:65
> > +#if !ENABLE(NOTIFICATIONS) && !ENABLE(LEGACY_NOTIFICATIONS)
> 
> I don't like to use unnecessary macro. Probably it looks Ossy wants to use a
> thing similar to below style. 
> 
> NotificationPermissionRequestManager::
> NotificationPermissionRequestManager(WebPage* page)
> {
> #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
>     m_page = page;
> #else
>     UNUSED_PARAM(page);
> #endif
> }

haha, you must be a thought-reader :)
Comment 9 Gyuyoung Kim 2015-04-15 09:03:26 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > Comment on attachment 250791 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=250791&action=review
> > 
> > > Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:65
> > > +#if !ENABLE(NOTIFICATIONS) && !ENABLE(LEGACY_NOTIFICATIONS)
> > 
> > I don't like to use unnecessary macro. Probably it looks Ossy wants to use a
> > thing similar to below style. 
> > 
> > NotificationPermissionRequestManager::
> > NotificationPermissionRequestManager(WebPage* page)
> > {
> > #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
> >     m_page = page;
> > #else
> >     UNUSED_PARAM(page);
> > #endif
> > }
> 
> haha, you must be a thought-reader :)

Because you and I have dealt with similar issues many times. :)
Comment 10 Sungmann Cho 2015-04-15 09:22:57 PDT
(In reply to comment #7)
> Comment on attachment 250791 [details]
> Patch
> 
> Ouch, it's so ugly. :-/ Unfortunately we can't put UNUSED_PARAM to the same
> place as init list.
> 
> I think this one would be the least ugly solution
> 
> NotificationPermissionRequestManager::
> NotificationPermissionRequestManager(WebPage* page)
> {
> #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
>     m_page = page
> #else
>     UNUSED_PARAM(page);
> #endif
> }

Agreed. But the member initialization is slightly different from the assignment.
Any thoughts on this?
Comment 11 Sungmann Cho 2015-04-15 09:34:32 PDT
Created attachment 250795 [details]
Patch
Comment 12 Gyuyoung Kim 2015-04-15 09:56:31 PDT
(In reply to comment #10)
> (In reply to comment #7)
> > Comment on attachment 250791 [details]
> > Patch
> > 
> > Ouch, it's so ugly. :-/ Unfortunately we can't put UNUSED_PARAM to the same
> > place as init list.
> > 
> > I think this one would be the least ugly solution
> > 
> > NotificationPermissionRequestManager::
> > NotificationPermissionRequestManager(WebPage* page)
> > {
> > #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
> >     m_page = page
> > #else
> >     UNUSED_PARAM(page);
> > #endif
> > }
> 
> Agreed. But the member initialization is slightly different from the
> assignment.
> Any thoughts on this?

I think biggest difference is generally that copy constructor of *WebPage* class is called or not. In this case, however, m_page is a pointer type of *WebPage*. Thus it seems to me that there is no big difference between both.
Comment 13 Sungmann Cho 2015-04-15 11:36:11 PDT
Addressed your comments.(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > Comment on attachment 250791 [details]
> > > Patch
> > > 
> > > Ouch, it's so ugly. :-/ Unfortunately we can't put UNUSED_PARAM to the same
> > > place as init list.
> > > 
> > > I think this one would be the least ugly solution
> > > 
> > > NotificationPermissionRequestManager::
> > > NotificationPermissionRequestManager(WebPage* page)
> > > {
> > > #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
> > >     m_page = page
> > > #else
> > >     UNUSED_PARAM(page);
> > > #endif
> > > }
> > 
> > Agreed. But the member initialization is slightly different from the
> > assignment.
> > Any thoughts on this?
> 
> I think biggest difference is generally that copy constructor of *WebPage*
> class is called or not. In this case, however, m_page is a pointer type of
> *WebPage*. Thus it seems to me that there is no big difference between both.

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > Comment on attachment 250791 [details]
> > > Patch
> > > 
> > > Ouch, it's so ugly. :-/ Unfortunately we can't put UNUSED_PARAM to the same
> > > place as init list.
> > > 
> > > I think this one would be the least ugly solution
> > > 
> > > NotificationPermissionRequestManager::
> > > NotificationPermissionRequestManager(WebPage* page)
> > > {
> > > #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
> > >     m_page = page
> > > #else
> > >     UNUSED_PARAM(page);
> > > #endif
> > > }
> > 
> > Agreed. But the member initialization is slightly different from the
> > assignment.
> > Any thoughts on this?
> 
> I think biggest difference is generally that copy constructor of *WebPage*
> class is called or not. In this case, however, m_page is a pointer type of
> *WebPage*. Thus it seems to me that there is no big difference between both.

I addressed your comments. Thanks!
Comment 14 Darin Adler 2015-04-16 09:41:11 PDT
Comment on attachment 250795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250795&action=review

I’m OK with this as is, but it doesn’t really seem like much of an improvement.

> Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:67
>  NotificationPermissionRequestManager::NotificationPermissionRequestManager(WebPage* page)
> +{
>  #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
> -    : m_page(page)
> +    m_page = page;
> +#else
> +    UNUSED_PARAM(page);
>  #endif
> -{
> -    (void)page;
>  }

I don’t really like the way this changes from initialization syntax to assignment syntax. I’d prefer to avoid both UNUSED_PARAM and (void) myself by just repeating the function twice:

#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)

NotificationPermissionRequestManager::NotificationPermissionRequestManager(WebPage* page)
    : m_page(page)
{
}

#else

NotificationPermissionRequestManager::NotificationPermissionRequestManager(WebPage*)
{
}

#endif

One nice thing about that is that it clears the way for further refinement, for example, we could make the empty constructor inline by putting it in the header and marking it inline without making the non-empty constructor be inlined.
Comment 15 Sungmann Cho 2015-04-16 17:30:33 PDT
Created attachment 250982 [details]
Patch
Comment 16 Sungmann Cho 2015-04-16 17:34:46 PDT
Comment on attachment 250982 [details]
Patch

Address Darin's comments
Comment 17 WebKit Commit Bot 2015-04-16 18:26:11 PDT
Comment on attachment 250982 [details]
Patch

Clearing flags on attachment: 250982

Committed r182936: <http://trac.webkit.org/changeset/182936>
Comment 18 WebKit Commit Bot 2015-04-16 18:26:16 PDT
All reviewed patches have been landed.  Closing bug.