On Fri, 11 Jan 2019, Ted Unangst wrote:
> Stefan Fritsch wrote:
> > +#define CREAD(sc, memb)
> > \
> > + ({
> > \
> > + struct virtio_pci_common_cfg c;
> > \
> > + size_t off = offsetof(struct virtio_pci_common_cfg, memb);
> > \
> > + size_t size = sizeof(c.memb);
> > \
> > + typeof(c.memb) val;
> > \
> > +
> > \
> > + switch (size) {
> > \
> > + case 1:
> > \
> > + val = bus_space_read_1(sc->sc_iot, sc->sc_ioh, off);
> > \
> > + break;
> > \
> > + case 2:
> > \
> > + val = bus_space_read_2(sc->sc_iot, sc->sc_ioh, off);
> > \
> > + break;
> > \
> > + case 4:
> > \
> > + val = bus_space_read_4(sc->sc_iot, sc->sc_ioh, off);
> > \
> > + break;
> > \
> > + case 8:
> > \
> > + val = bus_space_read_8(sc->sc_iot, sc->sc_ioh, off);
> > \
> > + break;
> > \
> > + }
> > \
> > + DNPRINTF(2, "%s: %d: off %#zx size %#zx read %#llx\n",
> > \
> > + __func__, __LINE__, off, size, (unsigned long long)val);
> > \
> > + val;
> > \
> > + })
>
> I'm not a big fan of statement expressions. I thought we purged most of them
> from the tree in fact. I would say just use an inline. And no typeof.
>
> (There's only one typeof in sys/ outside of drm, and I added it, but no more
> please.)
>
A simple inline function does not work because memb is the name of a
member of a struct. But combined with a macro it would work. Like this:
/* only used for sizeof, not actually allocated */
extern struct virtio_pci_common_cfg ccfg;
static inline
uint64_t _cread(struct virtio_pci_softc *sc, unsigned off, unsigned size)
{
uint64_t val;
switch (size) {
case 1:
val = bus_space_read_1(sc->sc_iot, sc->sc_ioh, off);
break;
case 2:
val = bus_space_read_2(sc->sc_iot, sc->sc_ioh, off);
break;
case 4:
val = bus_space_read_4(sc->sc_iot, sc->sc_ioh, off);
break;
case 8:
val = bus_space_read_8(sc->sc_iot, sc->sc_ioh, off);
break;
}
return val;
}
#define CREAD(sc, memb) _cread(sc, \
offsetof(struct virtio_pci_common_cfg, memb), sizeof(ccfg.memb))
The compiler should optimize this to the same code as the complicated
macro above. You think this variant is acceptable?
Cheers,
Stefan
