On Thu, 26.12.13 10:09, Russ Allbery (r...@debian.org) wrote: > > Zbigniew Jędrzejewski-Szmek <zbys...@in.waw.pl> writes: > > > As discussed on IRC, here's a patch which takes the simple approach of > > allowing globbing on loaded unit names in various (almost all :)) systemctl > > commands. Comments? > > I don't know the coding style of systemd, so please feel free to ignore > this comment if it's not consistent with the style used elsewhere. But > since I saw this in passing: > > > diff --git a/src/core/device.c b/src/core/device.c > > index 2c44dec..d3976c9 100644 > > --- a/src/core/device.c > > +++ b/src/core/device.c > > @@ -268,7 +268,7 @@ static int device_update_unit(Manager *m, struct > > udev_device *dev, const char *p > > memcpy(e, w, l); > > e[l] = 0; > > > > - n = unit_name_mangle(e); > > + n = unit_name_mangle(e, false); > > if (!n) { > > r = -ENOMEM; > > goto fail; > > A wise person convinced me a while back to avoid boolean arguments to C > functions in most cases because the meaning of the argument is very > inobvious at the call site. "false" what? What's disabled? One has to > go read the function definition to know, and while that's easy to find, > particularly with a cross-referencing editor, it takes one more step. > > What I've switched to instead is using tiny enums for this purpose. So: > > enum mangle_type { > MANGLE_NOGLOB = 0, > MANGLE_GLOB = 1 > }; > > and then at the call site: > > n = unit_name_mangle(e, MANGLE_NOGLOB); > > which makes the meaning of that argument immediately obvious.
As long as this is just one boolean arg on functions, or the functions are internally used I think booleans are fine to use. I don't think flag fields are necessarily the best choice for APIs in general. For APIs that are built around a context object seperate boolean setter calls are the better choice (i.e. foobar_set_waldo() to set som boolean called "waldo" on a context object "foobar). Then in many APIs we exposed we actually return strings instead of enums, since that's much easier to extend. (libudev does this for example, and so does libsystemd-login) So, the summary is: keep it low on booleans, keep it low on flags. The middle ground where you don't have much of neither is the best, and in general having fewer options is better than having many... Lennart -- Lennart Poettering, Red Hat -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org