Created attachment 64676 [details] approximate fix (probably more macros to adapt) Version: 3.7 SVN OS: Linux At least with "clang version 3.1 (trunk 142234)" built from trunk, valgrind.h and memcheck.h from trunk valgrind (rev. 12163, towards 3.7.0) produce "expression result unused [-Wunused-value]" warnings when client code uses certain macros that are intended to not actually return values. Experienced this while building a recent LibreOffice (towards 3.5) source tree (before <http://cgit.freedesktop.org/libreoffice/core/commit/?id=e2934c43102eb6e6f46b238276fbe3b0274699a8> "sal: workaround warnings in valgrind macros" got committed) in sal/rtl/source/alloc_cache.c. The attached valgrind-warnings.patch silences exactly those macros I experienced problems with, but there are probably more that follow the same pattern. Reproducible: Didn't try Steps to Reproduce: compile code calling VALGRIND_CREATE_MEMPOOL with recent trunk clang Actual Results: "expression result unused [-Wunused-value]" warning Expected Results: no warning
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.