Re: [PATCH 3/4] packed-backend: extract munmap operation for `MMAP_TEMPORARY`

shejialuo <shejialuo@gmail.com> writes:

In general, a refactoring tomove conditionals like this to the callee and make the callers unconditionally call the helper is an antipattern from maintainability's point of view.

Imagine what would happen when we acquire a different mmap_strategy in the future, and by that time, there are callers in the codebase other than what we currently have (which is just one below after this patch). Do you have to verify if all existing callers that trusted "if_temporary" really meant MMAP_TEMPORARY, as the name of the helper function suggested, or do some of the callers meant "any strategy other than MMAP_OK"? What if some callers want the former and others want the latter semantics?

Without even talking about longer term maintainability, at the callsite, _if_temporary in the name is a much weaker sign than an explicit if() condition that says what is going on.

I'd prefer the caller to be more like

	if (mmap_strategy == MMAP_TEMPORARY)
		munmap_temporary_stapshot(snapshot)

and make the caller to return immediately when !snapshot, i.e.

	static void munmap_temporary_stapshot(struct snapshot *snapshot
	{
		if (!snapshot)
			return;
		... the rest of the helper function ...
	}

Thanks.