Le lundi 24 mai 2021, 14:36:14 CEST Đoàn Trần Công Danh a écrit : > On 2021-05-24 10:41:18+0100, Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > > Hi Lénaïc > > > > On 24/05/2021 08:15, Lénaïc Huard wrote: > > > The first parameter of `XXX_update_schedule` and alike functions is a > > > boolean specifying if the tasks should be scheduled or unscheduled. > > > > > > Using an `enum` with `ENABLE` and `DISABLE` values can make the code > > > clearer. > > > > I'm sorry to say that I'm not sure this does make the code clearer overall > > - I wish I'd spoken up when Danh suggested it. > > While > > > > launchctl_boot_plist(DISABLE, filename, cmd) > > > > is arguably clearer than > > > > launchctl_boot_plist(0, filename, cmd) > > > > we end up with bizarre tests like > > > > if (enabled == ENABLED) > > > > rather than > > > > if (enabled) > > > > and in the next patch we have > > > > (enable == ENABLE && (opts->scheduler == i)) ? > > > > ENABLE : DISABLE; > > > > rather than > > > > enable && opts->scheduler == i > > > > Also looking at the next patch it seems as this one is missing some > > conversions in maintenance_start() as it is still calling > > update_background_schedule() with an integer rather than the new enum. > > Yes, in this form, I also think the change looks bizarre. > And, it's entirely my fault. > > I also agree with Ævar that 0 and 1 is meant well for off/on. > > However, I still think > > launchctl_boot_plist(0, filename, cmd) > > would require some degree on code navigation to figure out what would > that LoC does. > > I'm thinking about rename the function. But, it would trigger a forever > bikeshedding, which shouldn't be a blocker for this series. > > > I'd be happy to see this being dropped I'm afraid > > So, let's drop this patch and start a new conversation when the dust > settled. Hi, I think the reason why the code looks worse is because I used an enum and I didn’t want to make any assumption about how the enum members would be evaluated in a boolean context. Do you think it would make sense to drop the enum type, to revert all logic changes (Use `if (enabled)` back instead of `switch`, etc.), and to define the following constants : static const int DISABLE = 0; static const int ENABLE = 1; so that we can keep function invocation in the form of `launchctl_boot_plist(DISABLE, filename, cmd)` ?
