* Wei Yang <richard.weiyang@xxxxxxxxx> [240808 21:59]: > On Thu, Aug 08, 2024 at 12:53:55PM -0400, Liam R. Howlett wrote: > >* Wei Yang <richard.weiyang@xxxxxxxxx> [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() This is used much lower in the stack, where you see the entry is coming in and it seems obvious what is going on. > * mt_is_reserved() reading that something is reserved makes sense. Saying an entry is an internal tree node makes no sense, it's also buried in a function that doesn't immediately make sense to be branching on a value. > > 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? The confusing point is the resulting code doesn't make this any more friendly for the audience, why are we expanding the root node on mt_is_internal(entry)? Why is entry an internal node? How do we get to this function with a node as an entry - That seems wrong, let's waste time digging into how that happened.. You didn't make it more friendly, you made it more puzzling. > > 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(). I'm all for readable code - in fact, I'd be more inclined to accept a static inline function that just does the check with a comment about what it's doing.. > > To be honest, an open coded check doesn't looks a good practice. Thank you for your valuable insight. The entire point of your patch is to make it more readable and it does not do that. If you want to make it more readable, then I will look at that patch, but I don't particularly feel motivated to spend time doing that after this exchange. Liam
