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

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 [off-list ref] wrote:

quoted

Hi Lénaïc

On 24/05/2021 08:15, Lénaïc Huard wrote:

quoted

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.

quoted

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)` ?