Bug 44870

Summary: Modify ASSERT_UNUSED and UNUSED_PARAM similar to Qt's Q_UNUSED
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, webkit-ews
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 44593    
Attachments:
Description Flags
proposed patch
none
proposed patch
none
proposed patch
none
proposed patch darin: review+

Description Csaba Osztrogonác 2010-08-30 07:29:45 PDT
Q_UNUSED is more specific than a simple (void)x ,
so ASSERT_UNUSED should call Q_UNUSED for Qt builds
to make ICC and RVCT happy.

ASSERT_UNUSED:
--------------
#define ASSERT_UNUSED(variable, assertion) ((void)variable)

Q_UNUSED:
---------
#if defined(Q_CC_INTEL) && !defined(Q_OS_WIN) || defined(Q_CC_RVCT)
template <typename T>
inline void qUnused(T &x) { (void)x; }
#  define Q_UNUSED(x) qUnused(x);
#else
#  define Q_UNUSED(x) (void)x;
#endif
Comment 1 Csaba Osztrogonác 2010-08-30 07:36:16 PDT
Created attachment 65912 [details]
proposed patch

I didn't test it locally, please let EWS to do it before review.
Comment 2 Antonio Gomes 2010-08-30 07:42:00 PDT
Comment on attachment 65912 [details]
proposed patch

r=me

Please set cq+ or manually checkin when bots gets green.
Comment 3 Early Warning System Bot 2010-08-30 07:55:15 PDT
Attachment 65912 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3815170
Comment 4 Csaba Osztrogonác 2010-08-30 08:05:16 PDT
Comment on attachment 65912 [details]
proposed patch

patch is incorrect, fix is coming soon
Comment 5 Csaba Osztrogonác 2010-08-30 08:21:28 PDT
Created attachment 65915 [details]
proposed patch

- include added
- incorrect paranthesis removed
- tested locally :)
Comment 6 Darin Adler 2010-08-30 09:03:53 PDT
What about UNUSED_PARAM? What about non-Qt platforms?

I’d like to see both ASSERT_UNUSED and UNUSED_PARAM use the best technique available. Probably they would just do it directly instead of using Q_UNUSED.
Comment 7 Csaba Osztrogonác 2010-08-30 09:45:21 PDT
Created attachment 65925 [details]
proposed patch

UNUSED_PARAM use Q_UNUSED too
Comment 8 Csaba Osztrogonác 2010-08-30 09:48:08 PDT
(In reply to comment #6)
> What about UNUSED_PARAM? What about non-Qt platforms?
> 
> I’d like to see both ASSERT_UNUSED and UNUSED_PARAM use the best technique available. Probably they would just do it directly instead of using Q_UNUSED.

Do you mean we should copy and use the logic of Q_UNUSED instead of using Q_UNUSED?
Comment 9 Eric Seidel (no email) 2010-08-30 10:10:35 PDT
Attachment 65925 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3870149
Comment 10 Darin Adler 2010-08-30 10:12:11 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > What about UNUSED_PARAM? What about non-Qt platforms?
> > 
> > I’d like to see both ASSERT_UNUSED and UNUSED_PARAM use the best technique available. Probably they would just do it directly instead of using Q_UNUSED.
> 
> Do you mean we should copy and use the logic of Q_UNUSED instead of using Q_UNUSED?

Yes. We’d want to do that if there are any non-Qt ports that also work with the compilers that require the alternate technique. And if we did that, then we could avoid having any Qt-specific code.

In the end, I don’t care whether we use Q_UNUSED or not, I just want the macros to work well for everyone.
Comment 11 Csaba Osztrogonác 2010-08-31 02:36:55 PDT
Created attachment 66023 [details]
proposed patch

I refactored ASSERT_UNUSED and UNUSED_PARAM
to have similar behavior as Qt's Q_UNUSED.

If this patch will be landed, we 
won't need to use Q_UNUSED anymore.
Comment 12 Darin Adler 2010-08-31 08:11:15 PDT
Comment on attachment 66023 [details]
proposed patch

Formatting isn’t quite right. We normally put the "&" next to the "T". We normally don’t put a space after the keyword "template". And the names of the functions aren't quite right either. But I think we can live with it like this, so r=me
Comment 13 Csaba Osztrogonác 2010-08-31 09:56:10 PDT
(In reply to comment #12)
> (From update of attachment 66023 [details])
> Formatting isn’t quite right. We normally put the "&" next to the "T". We normally don’t put a space after the keyword "template". And the names of the functions aren't quite right either. But I think we can live with it like this, so r=me

I fixed style errors ( T& and template<...> ), and 
then landed it: http://trac.webkit.org/changeset/66489