Re: [PATCH v4 2/4] maintenance: introduce ENABLE/DISABLE for code clarity

["Lénaïc Huard" <lenaic@lhuard.fr>]

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@gmail.com> 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)` ?