Bug 143750 – Use UNUSED_PARAM instead of the void casting to suppress unused parameter warnings

• Component: WebKit2
• Status: RESOLVED FIXED

[Sungmann Cho]

Use UNUSED_PARAM instead of the void casting.

[Sungmann Cho]

Created attachment 250780 [details]
Patch

[Sungmann Cho]

Created attachment 250781 [details]
Patch

[Csaba Osztrogonác]

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)

[Sungmann Cho]

Created attachment 250791 [details]
Patch

[Sungmann Cho]

Thank you for pointing that out. I updated the patch.
Please take a look. Thanks!

[Gyuyoung Kim]

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
}

[Csaba Osztrogonác]

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
}

[Csaba Osztrogonác]

(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 :)

[Gyuyoung Kim]

(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. :)

[Sungmann Cho]

(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?

[Sungmann Cho]

Created attachment 250795 [details]
Patch

[Gyuyoung Kim]

(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.

[Sungmann Cho]

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!

[Darin Adler]

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.

[Sungmann Cho]

Created attachment 250982 [details]
Patch

[Sungmann Cho]

Comment on attachment 250982 [details]
Patch

Address Darin's comments

[WebKit Commit Bot]

Comment on attachment 250982 [details]
Patch

Clearing flags on attachment: 250982

Committed r182936: <http://trac.webkit.org/changeset/182936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.