[lttng-dev] [PATCH 1/3] urcu: avoid "expression result unused" from clang

[Mathieu Desnoyers]

* Mathieu Desnoyers (mathieu.desnoyers at efficios.com) wrote:
> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> > On 12/08/2012 11:00 PM, Mathieu Desnoyers wrote:
> > > * Lai Jiangshan (eag0628 at gmail.com) wrote:
> > >> How about the one in yy.diff?
> > >>
> > >> xx.diff fixes the one what I had sent in 1/3 patch, but it may still
> > >> cause warning in future if the complier become more strict.
> > > 
> > > Hrm. In any cases, I think that even with the 2 patches you propose, the
> > > warning "unused expression return" would still be valid (because we end
> > > up not using the result of the outer statement-expression). 
> > 
> > The one in yy.diff:
> > 
> > 1 ? _v : statement_which_has_side_affect;
> > 
> > The compiler should not warn for it. "?:" can be used for control flow,
> > the complier should only warn if both branches are warning.
> 
> In the case of yy.diff, whether the warning will appear or not would
> depend on the phase at which the compiler generates the warning, no ?
> For instance, it it chooses to check for unused return values after
> eliminating dead code (considering branches that cannot be taken), then
> we would be in the same situation as we are today, even if we have a
> statement with side effect.
> 
> I am more and more enclined to try asking their opinion to the clang
> community about this.

I finally found a way to silence warnings both in gcc and clang:


diff --git a/urcu/system.h b/urcu/system.h
index 2a45f22..6f31459 100644
--- a/urcu/system.h
+++ b/urcu/system.h
@@ -47,11 +47,11 @@
  * Store v into x, where x is located in shared memory. Performs the
  * required cache flush after writing. Returns v.
  */
-#define CMM_STORE_SHARED(x, v)		\
-	({				\
-		__typeof__(x) _v = _CMM_STORE_SHARED(x, v);	\
-		cmm_smp_wmc();		\
-		_v;			\
+#define CMM_STORE_SHARED(x, v)						\
+	({								\
+		__typeof__(x) _v = _CMM_STORE_SHARED(x, v);		\
+		cmm_smp_wmc();						\
+		_v = _v;	/* Work around clang "unused result" */	\
 	})
 
 #endif /* _URCU_SYSTEM_H */

Thanks,

Mathieu


> 
> Thanks,
> 
> Mathieu
> 
> > 
> > 
> > We might
> > > want to consider simpler approaches:
> > > 
> > > 1 - Add the missing (void) casts before each CMM_STORE_SHARED() in the
> > >     code. I don't like this because it makes the CMM_STORE_SHARED common
> > >     case more cumbersome to use.
> > > 2 - Change the CMM_STORE_SHARED() API so it does not return anything.
> > >     I think most use-cases don't involve using its return value anyway.
> > > 
> > > Thoughts ?
> > > 
> > > Thanks,
> > > 
> > > Mathieu
> > > 
> > >>
> > >> On Sat, Dec 8, 2012 at 1:27 AM, Lai Jiangshan <eag0628 at gmail.com> wrote:
> > >>>
> > >>>
> > >>> On Saturday, December 8, 2012, Mathieu Desnoyers wrote:
> > >>>>
> > >>>> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> > >>>>> The last expression result is unused and clang will complain.
> > >>>>> The trick in the patch supresses the complaint
> > >>>>
> > >>>> Hrm, but with this patch, gcc complains:
> > >>>>
> > >>>> rculfhash.c:1921:3: warning: variable '_w' set but not used
> > >>>> [-Wunused-but-set-variable]
> > >>>>
> > >>>> using
> > >>>>
> > >>>> gcc version 4.7.2 (Debian 4.7.2-4)
> > >>>
> > >>>
> > >>> Ouch, my gcc is too old
> > >>>
> > >>>
> > >>>>
> > >>>>
> > >>>> Thoughts ?
> > >>>>
> > >>>> Thanks,
> > >>>>
> > >>>> Mathieu
> > >>>>
> > >>>>>
> > >>>>> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
> > >>>>> ---
> > >>>>>  urcu/system.h        |    5 ++++-
> > >>>>>  2 files changed, 4 insertions(+), 1 deletions(-)
> > >>>>>
> > >>>>> diff --git a/urcu/system.h b/urcu/system.h
> > >>>>> index 2a45f22..6b7b0af 100644
> > >>>>> --- a/urcu/system.h
> > >>>>> +++ b/urcu/system.h
> > >>>>> @@ -46,12 +46,15 @@
> > >>>>>  /*
> > >>>>>   * Store v into x, where x is located in shared memory. Performs the
> > >>>>>   * required cache flush after writing. Returns v.
> > >>>>> + * "_w" here avoids the warning from clang:
> > >>>>> + *   warning: expression result unused [-Wunused-value]
> > >>>>>   */
> > >>>>>  #define CMM_STORE_SHARED(x, v)               \
> > >>>>>       ({                              \
> > >>>>>               __typeof__(x) _v = _CMM_STORE_SHARED(x, v);     \
> > >>>>> +             __typeof__(x) _w;       \
> > >>>>>               cmm_smp_wmc();          \
> > >>>>> -             _v;                     \
> > >>>>> +             _w = _v;                \
> > >>>>>       })
> > >>>>>
> > >>>>>  #endif /* _URCU_SYSTEM_H */
> > >>>>> --
> > >>>>> 1.7.4.4
> > >>>>>
> > >>>>>
> > >>>>> _______________________________________________
> > >>>>> lttng-dev mailing list
> > >>>>> lttng-dev at lists.lttng.org
> > >>>>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> > >>>>
> > >>>>
> > >>>> --
> > >>>> Mathieu Desnoyers
> > >>>> Operating System Efficiency R&D Consultant
> > >>>> EfficiOS Inc.
> > >>>> http://www.efficios.com
> > >>>>
> > >>>> _______________________________________________
> > >>>> lttng-dev mailing list
> > >>>> lttng-dev at lists.lttng.org
> > >>>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> > > 
> > > 
> > > 
> > > 
> > 
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com