On 3/21/22 9:52 PM, Mariusz Tkaczyk wrote:
On Sat, 19 Mar 2022 23:12:36 +0800
Coly Li <colyli@xxxxxxx> wrote:
Hi Kinga,
I am fine with the above idea, except the extra is_container(). IMHO
comparing directly with LEVEL_CONTAINER is fine, adding
is_container() doesn't help too much.
Hi Coly,
It is used in many places and is long. This is the main reason we
changed that. As you can see, some ifs was simplified and readability
is improved.
Generally, if something is repeated two or more times, it should be
gathered inside function. This simplifies future changes (for example
if we decide to add other level check here). Also it simplifies code
analysis (IDE will show us all usages) and it gives basic description.
In this case it could be redundant but IMO it is a design we should
follow.
We already proposed similar inline for raid456 in other patch.
Hmm, using is_container() may reduce 5 characters, and there are around
19 replacement and reduces around 95~100 characters, but introducing
is_container() increases around 280+ characters. This is why I am not
able to fully support the is_container() replacement.
Except for the is_container() replacement, I am OK with the problem
fixed. Could you please divide the original patch into 2 parts, one is
adding the LEVEL_CONTAINER checking, one is the is_container()
replacement, then I can be supportive to the fix itself, and conserve my
opinion to the is_container() replacement.
Thanks.
Coly Li
