Messages in this thread | | | Date | Tue, 12 Dec 2023 20:57:48 +0000 | From | Matthew Wilcox <> | Subject | Re: [PATCH] maple_tree: do not preallocate nodes for slot stores |
| |
On Tue, Dec 12, 2023 at 11:46:40AM -0800, Sidhartha Kumar wrote: > + /* Slot store, does not require additional nodes */ > + if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree)) > + || (wr_mas.offset_end - mas->offset == 1))) > + return 0;
Should we refactor this into a mas_is_slot_store() predicate?
A few coding-style problems with it as it's currently written:
1. The indentation on the second line is wrong. It makes the continuation of the condition look like part of the statement. Use extra whitespace to indent. eg:
if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree)) || (wr_mas.offset_end - mas->offset == 1))) return 0;
2. The operator goes last on the line, not at the beginning of the continuation line. ie:
if ((node_size == mas->end) && ((!mt_in_rcu(mas->tree)) || (wr_mas.offset_end - mas->offset == 1))) return 0;
3. You don't need parens around the !mt_in_rcu(mas->tree). There's no ambiguity to solve here:
if ((node_size == mas->end) && (!mt_in_rcu(mas->tree) || (wr_mas.offset_end - mas->offset == 1))) return 0;
But I'd write it as:
if ((node_size == mas->end) && (!mt_in_rcu(mas->tree) || (wr_mas.offset_end - mas->offset == 1))) return 0;
because then the whitespace matches how you're supposed to parse the condition, and so the next person to read this code will have an easier time of it.
| |