Re: [Xen-devel] [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_* [and 1 more messages]

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type 
argument to xc_psr_*"):
> On Thu, Apr 28, 2016 at 06:29:03PM +0100, Ian Jackson wrote:
> > I'm very much against introducing casts which are not absolutely
> > necessary.  Casts are a big hammer which can suppress important
> > warnings (such as conversions between integers and pointers).
> > 
> > This anomaly with the same enum defined in two places with two names
> > is pretty poor.  But if we are to perpetuate it, as perhaps we must,
> > then rather than casting at each conversion point, we should introduce
> > an inline function which contains the cast.  That way each call site
> > remains more typesafe.
> 
> The two enums aren't going away any time soon.

Sadly, I think you're right.

> Does the following diff meet your requirement?

Yes, that is exactly the kind of thing I was thinking of.  It makes
the cast non-routine by putting it in a dedicated function whose
authors/readers know to check it's right.

Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

(with a suitable commit message)

Roger Pau Monne writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the 
type argument to xc_psr_*"):
> On Thu, Apr 28, 2016 at 09:49:11PM +0100, Wei Liu wrote:
> > +    BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
> > +    return (xc_psr_cat_type)type;
> 
> In order to prevent using a cast, we could use a union:
> 
> union {
>     libxl_psr_cbm_type libxl_psr;
>     xc_psr_cat_type xc_psr;
> } u;
> 
> u.libxl_psr = type;
> return u.xc_psr;

No, not really.

Firstly, although we compile with -fno-strict-aliasing, this is a bad
example in these days of hostile optimisers: without
-fno-strict-aliasing, the compiler is allowed to assume that the loads
and stores are to different variables, even though the contrary is
evident.

Secondly, it's clumsy.

Thirdly, this kind of union aliasing is normally used for
representation (structure) punning.


Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel