Bug 44870 – Modify ASSERT_UNUSED and UNUSED_PARAM similar to Qt's Q_UNUSED

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

Created attachment 65912 [details]
proposed patch

I didn't test it locally, please let EWS to do it before review.

[Antonio Gomes]

Comment on attachment 65912 [details]
proposed patch

r=me

Please set cq+ or manually checkin when bots gets green.

[Early Warning System Bot]

Attachment 65912 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3815170

[Csaba Osztrogonác]

Comment on attachment 65912 [details]
proposed patch

patch is incorrect, fix is coming soon

[Csaba Osztrogonác]

Created attachment 65915 [details]
proposed patch

- include added
- incorrect paranthesis removed
- tested locally :)

[Darin Adler]

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.

[Csaba Osztrogonác]

Created attachment 65925 [details]
proposed patch

UNUSED_PARAM use Q_UNUSED too

[Csaba Osztrogonác]

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

[Eric Seidel (no email)]

Attachment 65925 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3870149

[Darin Adler]

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

[Csaba Osztrogonác]

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.

[Darin Adler]

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

[Csaba Osztrogonác]

(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