Re: [PATCH] maple_tree: use xa_is_internal() for better reading

[Wei Yang <richard.weiyang@gmail.com>]

On Thu, Aug 08, 2024 at 12:53:55PM -0400, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [240808 00:38]:
>> If entry is a special case, we need to expand root to store it. This
>> case is exactly the case of xa_is_internal().
>> 
>> Let's use xa_is_internal() for the check, which is friendly for
>> audience.
>
>Nack
>
>This check is to see if it will be detected as an internal node - this
>much is correct.
>
>But changing the check to use this function reads far worse than what
>exists today.  If you look at the code below, it seems to indicate that
>the internal entry is being inserted into the tree - but it's not an
>internal entry, it's an entry that would be detected as an internal
>entry.
>

From my understanding, you want to say xa_is_internal() only applies on "an
entry" in the tree instead of a "value" we want to insert into the tree.

But it looks current code already use xa_is_internal() to check the value we
want to insert in the tree.

For example:

  * xa_is_advanced()
  * mt_is_reserved()

xa_is_advanced() is used in mtree_insert_range() to prevent inserting an
advanced value for a normal API.
mt_is_reserved() is used in mtree_alloc_range() to prevent inserting reserved
value.

Both check apply to an entry which will be inserted, instead of an entry in
current tree. And they are clearly to inform me the value we want to insert
has special meaning for maple tree and need to be handled carefully.

>This is not friendlier for the audience, it's confusing.
>

The confusing point comes from audience would think the "entry" is already an
internal entry in the tree?

If you really think audience would be misled, I would suggest introduce a new
helper, e.g. mt_is_internal(), just like xa_is_advanced()/mt_is_reserved().

To be honest, an open coded check doesn't looks a good practice.

-- 
Wei Yang
Help you, Help me