Bug 284384

Wouldn't it be better to use do { ... } while(0) instead of (void)... ?

No, it would only move the warning into the do loop.

Sorry, I meant do { (void) ... } while (0). Also, there are more client requests that do not return a value than those updated via the attached patch.

Created attachment 64789 [details]
alternative fix

I don't have clang 3.1 available here. Does the "alternative fix" help ?

(In reply to comment #4)
> Created an attachment (id=64789) [details]
> alternative fix
> 
> I don't have clang 3.1 available here. Does the "alternative fix" help ?

Is it necessary to introduce VALGRIND_DO_CLIENT_REQUEST_VOID?
We could as well fix the issue like so:

 #define VALGRIND_CREATE_MEMPOOL(pool, rzB, is_zeroed)             \
     do { \ 
       (void) VALGRIND_DO_CLIENT_REQUEST_EXPR(0,                     \
                                VG_USERREQ__CREATE_MEMPOOL,        \
                                pool, rzB, is_zeroed, 0, 0) \
     while (0)

which is similar to what was proposed in the original patch plus an additional do...while(0) wrapper around it.

If somebody wants to explicitly ignore the value, she can write the cast herself or roll her own macro.

(In reply to comment #5)
> Is it necessary to introduce VALGRIND_DO_CLIENT_REQUEST_VOID?
> We could as well fix the issue like so:
> 
>  #define VALGRIND_CREATE_MEMPOOL(pool, rzB, is_zeroed)             \
>      do { \ 
>        (void) VALGRIND_DO_CLIENT_REQUEST_EXPR(0,                     \
>                                 VG_USERREQ__CREATE_MEMPOOL,        \
>                                 pool, rzB, is_zeroed, 0, 0) \
>      while (0)
> 
> which is similar to what was proposed in the original patch plus an additional
> do...while(0) wrapper around it.

As far as I can see the above is an inlined version of the definition based on VALGRIND_DO_CLIENT_REQUEST_VOID() ? The reason I introduced VALGRIND_DO_CLIENT_REQUEST_VOID() is because the pattern of ignoring the return value occurs so many times that the Valgrind header files are IMHO more readable by introducing VALGRIND_DO_CLIENT_REQUEST_VOID() than by directly introducing do { (void) ... } while (0) at many places.

(In reply to comment #6)
> (In reply to comment #5)
> > Is it necessary to introduce VALGRIND_DO_CLIENT_REQUEST_VOID?
> > We could as well fix the issue like so:
> > 
> >  #define VALGRIND_CREATE_MEMPOOL(pool, rzB, is_zeroed)             \
> >      do { \ 
> >        (void) VALGRIND_DO_CLIENT_REQUEST_EXPR(0,                     \
> >                                 VG_USERREQ__CREATE_MEMPOOL,        \
> >                                 pool, rzB, is_zeroed, 0, 0) \
> >      while (0)
> > 
> > which is similar to what was proposed in the original patch plus an additional
> > do...while(0) wrapper around it.
> 
> As far as I can see the above is an inlined version of the definition based on
> VALGRIND_DO_CLIENT_REQUEST_VOID() ? 

Yes, it is. I guess this is going to be a matter of personal preference. I find the version without macro more readable, even if it's more typing. But it's not enough of an issue to fight for it.

Created attachment 64807 [details]
alternative fix, v2

Renamed VALGRIND_DO_CLIENT_REQUEST_VOID() into VALGRIND_DO_CLIENT_REQUEST_STMNT(). Replaced even more occurrences of VALGRIND_DO_CLIENT_REQUEST_EXPR().

(In reply to comment #8)
> Created an attachment (id=64807) [details]
> alternative fix, v2
> 
> Renamed VALGRIND_DO_CLIENT_REQUEST_VOID() into
> VALGRIND_DO_CLIENT_REQUEST_STMNT(). Replaced even more occurrences of
> VALGRIND_DO_CLIENT_REQUEST_EXPR().

I like it. Much better naming than in the previous patch.

Yeah, I like it too.  Two small things though: can we have _STMT instead
of _STMNT ?  _EXPR and _STMT are the normal abbreviations, I think.  Secondly,
you say

+ * VALGRIND_DO_CLIENT_REQUEST_STMNT(): a statement that invokes a Valgrind
+ * client request that does not return a value.
+ *
  * VALGRIND_DO_CLIENT_REQUEST_EXPR(): a C expression that invokes a Valgrind
  * client request and whose value equals the client request result. Accepts
  * both pointers and integers as arguments.

It's clear that _STMT is executed for its side effects, since otherwise there
would be no point.  I think we should also say in the comments that _EXPR may
also have side effects (that's correct, right?) so there is no expectation 
that _EXPR denotes a pure function.

It would be good to get this in 3.7.0.  If you like I can make the above fixes
and commit it tomorrow (Monday).  LMK what you prefer to do.

(In reply to comment #10)
> Yeah, I like it too.  Two small things though: can we have _STMT instead
> of _STMNT ?  

+1

> _EXPR and _STMT are the normal abbreviations, I think.  

Yep, I've seen that in other projects, e.g. GCC.

Committed, r12226.  Thanks both.