A large part of the patches I'm going to attach are essentially expanding macros without touching at the expanded code. I'm attaching the script I used here. Use with `preprocess.py macro file`. The script doesn't care about formatting, so I ran clang-format after each iteration, which is why there's a patch in the queue that runs clang-format first. At the end of the 29 (!) patches, there's still some cleanup left to do, but I'd rather do them in a followup. This queue at least gets rid of *all* the macros in rb.h.

Note that "simplify the expanded code" usually means removing things like (this)-> after expansion.

Comment on attachment 8912541 [details] Bug 1403444 - Remove typedefs for RedBlackTrees. https://reviewboard.mozilla.org/r/183842/#review189126

Comment on attachment 8912542 [details] Bug 1403444 - Add getters and setters on RedBlackTreeNode. https://reviewboard.mozilla.org/r/183844/#review189132 ::: memory/build/rb.h:88 (Diff revision 1) > /* Node structure. */ > template <typename T> > -struct RedBlackTreeNode > +class RedBlackTreeNode > +{ > + uintptr_t mLeft; > + uintptr_t mRightAndColor; I feel like these would be better remaining as `T*`, because that's a better match for a pointer and a tagged pointer than `uintptr_t`. The operations on mLeft would lose a cast, those on mRight would gain a cast, but I think overall it would be slightly clearer. Also, please add a comment on mRightAndColor indicating that its lowest bit is the color bit.

Comment on attachment 8912543 [details] Bug 1403444 - Make the "static" part of what the rb_wrap macro expands to.. https://reviewboard.mozilla.org/r/183846/#review189134

Comment on attachment 8912544 [details] Bug 1403444 - Use a fixed size for the stack space used during rb_foreach. https://reviewboard.mozilla.org/r/183848/#review189136 ::: memory/build/rb.h:771 (Diff revision 1) > */ > > -#ifdef RB_NO_C99_VARARRAYS > - /* > +/* > - * Avoid using variable-length arrays, at the cost of using more stack space. > + * Avoid using variable-length arrays, at the cost of using more stack space. > - * Size the path arrays such that they are always large enough, even if a > + * Size the path arrays such that they are always large enough, even if a The comment about avoiding variable-length arrays doesn't really make sense now. Please reword appropriately. ::: memory/build/rb.h:783 (Diff revision 1) > - * is: > + * is: > - * > + * > - * (3 * ((SIZEOF_PTR<<3) - (SIZEOF_PTR_2POW+1))) > + * (3 * ((SIZEOF_PTR<<3) - (SIZEOF_PTR_2POW+1))) > - * > + * > - * This works out to a maximum depth of 87 and 180 for 32- and 64-bit > + * This works out to a maximum depth of 87 and 180 for 32- and 64-bit > - * systems, respectively (approximatly 348 and 1440 bytes, respectively). > + * systems, respectively (approximatly 348 and 1440 bytes, respectively). While you're here, "approximately" is misspelled.

Comment on attachment 8912545 [details] Bug 1403444 - Apply clang-format to the rb.h macros. https://reviewboard.mozilla.org/r/183850/#review189138 ::: memory/build/rb.h:844 (Diff revision 1) > - } \ > - } \ > + } \ > + } \ > + } \ > + } \ > + } \ > -} > + } The alignment of these closing braces is weird, but I guess all these macros will be gone by the end of the patch sequence, so it doesn't matter?

Comment on attachment 8912546 [details] Bug 1403444 - Abstract red-black-tree link field reference with a new macro. https://reviewboard.mozilla.org/r/183852/#review189142

Comment on attachment 8912547 [details] Bug 1403444 - Trivially expand rbp_left_get. https://reviewboard.mozilla.org/r/183854/#review189144

Comment on attachment 8912548 [details] Bug 1403444 - Trivially expand rbp_right_get. https://reviewboard.mozilla.org/r/183856/#review189146

Comment on attachment 8912549 [details] Bug 1403444 - Trivially expand rbp_red_get. https://reviewboard.mozilla.org/r/183858/#review189148

Comment on attachment 8912550 [details] Bug 1403444 - Trivially expand rbp_left_set. https://reviewboard.mozilla.org/r/183860/#review189150

Comment on attachment 8912552 [details] Bug 1403444 - Trivially expand rbp_{color,red,black}_set. https://reviewboard.mozilla.org/r/183864/#review189154 ::: memory/build/rb.h:320 (Diff revision 1) > bool rbp_ll_red; \ > rbp_rotate_left(a_type, a_field, (a_node), (r_node)); \ > rbp_ll_red = rb_node_field((a_node), a_field).IsRed(); \ > - rbp_color_set(a_type, a_field, (r_node), rbp_ll_red); \ > - rbp_red_set(a_type, a_field, (a_node)); \ > + rb_node_field((r_node), a_field) \ > + .SetColor(rbp_ll_red ? NodeColor::Red : NodeColor::Black); \ > + rb_node_field((a_node), a_field).SetColor(NodeColor::Red); \ The ternary operator comes up so often I wonder if having `SetColor(bool)` would be worthwhile. Also `SetBlack()` and `SetRed()`.

Comment on attachment 8912553 [details] Bug 1403444 - Replace some uses of IsRed() with Color() or IsBlack(). https://reviewboard.mozilla.org/r/183866/#review189156 Ok, so this patch at least partially reduces the need for the functions I suggested on the previous patch.

Comment on attachment 8912554 [details] Bug 1403444 - Move miscellaneous size related constants and macros earlier in mozjemalloc.cpp. https://reviewboard.mozilla.org/r/183868/#review189158

Comment on attachment 8912551 [details] Bug 1403444 - Trivially expand rbp_right_set. https://reviewboard.mozilla.org/r/183862/#review189160

Comment on attachment 8912542 [details] Bug 1403444 - Add getters and setters on RedBlackTreeNode. https://reviewboard.mozilla.org/r/183844/#review189132 > I feel like these would be better remaining as `T*`, because that's a better match for a pointer and a tagged pointer than `uintptr_t`. The operations on mLeft would lose a cast, those on mRight would gain a cast, but I think overall it would be slightly clearer. > > Also, please add a comment on mRightAndColor indicating that its lowest bit is the color bit. > The operations on mLeft would lose a cast, those on mRight would gain a cast, but I think overall it would be slightly clearer. Note that one operation on mRight gains two casts, and I'm not particularly convinced overall it makes things clearer. This is what this looks like after this treatment: template <typename T> class RedBlackTreeNode { T* mLeft; T* mRightAndColor; public: T* Left() { return mLeft; } void SetLeft(T* aValue) { mLeft = aValue; } T* Right() { return reinterpret_cast<T*>( reinterpret_cast<uintptr_t>(mRightAndColor) & uintptr_t(~1)); } void SetRight(T* aValue) { mRightAndColor = reinterpret_cast<T*>( (reinterpret_cast<uintptr_t>(aValue) & uintptr_t(~1)) | Color()); } NodeColor Color() { return static_cast<NodeColor>(reinterpret_cast<uintptr_t>(mRightAndColor) & 1); } bool IsRed() { return Color() == NodeColor::Red; } void SetColor(NodeColor aColor) { mRightAndColor = reinterpret_cast<T*>( (reinterpret_cast<uintptr_t>(mRightAndColor) & uintptr_t(~1)) | aColor); } };

Comment on attachment 8912542 [details] Bug 1403444 - Add getters and setters on RedBlackTreeNode. https://reviewboard.mozilla.org/r/183844/#review189132 > > The operations on mLeft would lose a cast, those on mRight would gain a cast, but I think overall it would be slightly clearer. > > Note that one operation on mRight gains two casts, and I'm not particularly convinced overall it makes things clearer. This is what this looks like after this treatment: > > template <typename T> > class RedBlackTreeNode > { > T* mLeft; > T* mRightAndColor; > > public: > T* Left() > { > return mLeft; > } > > void SetLeft(T* aValue) > { > mLeft = aValue; > } > > T* Right() > { > return reinterpret_cast<T*>( > reinterpret_cast<uintptr_t>(mRightAndColor) & uintptr_t(~1)); > } > > void SetRight(T* aValue) > { > mRightAndColor = reinterpret_cast<T*>( > (reinterpret_cast<uintptr_t>(aValue) & uintptr_t(~1)) | Color()); > } > > NodeColor Color() > { > return static_cast<NodeColor>(reinterpret_cast<uintptr_t>(mRightAndColor) & 1); > } > > bool IsRed() > { > return Color() == NodeColor::Red; > } > > void SetColor(NodeColor aColor) > { > mRightAndColor = reinterpret_cast<T*>( > (reinterpret_cast<uintptr_t>(mRightAndColor) & uintptr_t(~1)) | aColor); > } > }; Come to think of it, keeping integral types would even allow to use bit fields and remove half the manual bit fiddling.

Comment on attachment 8912545 [details] Bug 1403444 - Apply clang-format to the rb.h macros. https://reviewboard.mozilla.org/r/183850/#review189138 > The alignment of these closing braces is weird, but I guess all these macros will be gone by the end of the patch sequence, so it doesn't matter? > but I guess all these macros will be gone by the end of the patch sequence, so it doesn't matter? Indeed.

Comment on attachment 8912552 [details] Bug 1403444 - Trivially expand rbp_{color,red,black}_set. https://reviewboard.mozilla.org/r/183864/#review189154 > The ternary operator comes up so often I wonder if having `SetColor(bool)` would be worthwhile. Also `SetBlack()` and `SetRed()`. > The ternary operator comes up so often I wonder if having SetColor(bool) would be worthwhile. Also SetBlack() and SetRed(). As you've noted, most ternary operator uses are removed later on. At the end of the series, the remaining ones are unrelated to colors.

Comment on attachment 8912541 [details] Bug 1403444 - Remove typedefs for RedBlackTrees. Sorry, I messed up with mozreview when adding one more patch to the queue :( that reset the whole queue.

(In reply to Mike Hommey [:glandium] from comment #49) > Come to think of it, keeping integral types would even allow to use bit > fields and remove half the manual bit fiddling. FWIW, this is what this would look like: template <typename T> class RedBlackTreeNode { uintptr_t mLeft; uintptr_t mRight : sizeof(uintptr_t) * 8 - 1; NodeColor mColor : 1; public: T* Left() { return reinterpret_cast<T*>(mLeft); } void SetLeft(T* aValue) { mLeft = reinterpret_cast<uintptr_t>(aValue); } T* Right() { return reinterpret_cast<T*>(mRight << 1); } void SetRight(T* aValue) { mRight = reinterpret_cast<uintptr_t>(aValue) >> 1; } NodeColor Color() { return mColor; } bool IsBlack() { return Color() == NodeColor::Black; } bool IsRed() { return Color() == NodeColor::Red; } void SetColor(NodeColor aColor) { mColor = aColor; } }; Note mColor would need to be between mLeft and mRight for the current bit order to be preserved. (but only on little endian)

I'm inclined to stick with the old bit-masking approach. Seems more standard for tagged pointers, possibly because of the endianness issues.

Comment on attachment 8912541 [details] Bug 1403444 - Remove typedefs for RedBlackTrees. https://reviewboard.mozilla.org/r/183842/#review189440

Comment on attachment 8912943 [details] Bug 1403444 - Use templates for rb_node and rb_tree, and rename them. https://reviewboard.mozilla.org/r/184266/#review189442

Comment on attachment 8912944 [details] Bug 1403444 - Add getters and setters on RedBlackTreeNode. https://reviewboard.mozilla.org/r/184268/#review189444 ::: memory/build/rb.h:88 (Diff revision 1) > /* Node structure. */ > template <typename T> > -struct RedBlackTreeNode > +class RedBlackTreeNode > +{ > + uintptr_t mLeft; > + uintptr_t mRightAndColor; Still need comments here about these fields.

Comment on attachment 8912945 [details] Bug 1403444 - Make the "static" part of what the rb_wrap macro expands to.. https://reviewboard.mozilla.org/r/184270/#review189446

Comment on attachment 8912946 [details] Bug 1403444 - Use a fixed size for the stack space used during rb_foreach. https://reviewboard.mozilla.org/r/184272/#review189448

Comment on attachment 8912947 [details] Bug 1403444 - Apply clang-format to the rb.h macros. https://reviewboard.mozilla.org/r/184274/#review189450

Comment on attachment 8912948 [details] Bug 1403444 - Abstract red-black-tree link field reference with a new macro. https://reviewboard.mozilla.org/r/184276/#review189452

Comment on attachment 8912949 [details] Bug 1403444 - Trivially expand rbp_left_get. https://reviewboard.mozilla.org/r/184278/#review189454

Comment on attachment 8912950 [details] Bug 1403444 - Trivially expand rbp_right_get. https://reviewboard.mozilla.org/r/184280/#review189456

Comment on attachment 8912951 [details] Bug 1403444 - Trivially expand rbp_red_get. https://reviewboard.mozilla.org/r/184282/#review189458

Comment on attachment 8912952 [details] Bug 1403444 - Trivially expand rbp_left_set. https://reviewboard.mozilla.org/r/184284/#review189460

Comment on attachment 8912953 [details] Bug 1403444 - Trivially expand rbp_right_set. https://reviewboard.mozilla.org/r/184286/#review189462

Comment on attachment 8912954 [details] Bug 1403444 - Trivially expand rbp_{color,red,black}_set. https://reviewboard.mozilla.org/r/184288/#review189464

Comment on attachment 8912955 [details] Bug 1403444 - Replace some uses of IsRed() with Color() or IsBlack(). https://reviewboard.mozilla.org/r/184290/#review189466

Comment on attachment 8912956 [details] Bug 1403444 - Move miscellaneous size related constants and macros earlier in mozjemalloc.cpp. https://reviewboard.mozilla.org/r/184292/#review189468

Comment on attachment 8912957 [details] Bug 1403444 - Add methods to the RedBlackTree template class, replacing rb_wrap. https://reviewboard.mozilla.org/r/184294/#review189522 BTW, I'm making various style suggestions. It's fine to save them for later if doing them now would mess up the patch sequence. ::: memory/build/mozjemalloc.cpp:601 (Diff revision 1) > }; > -typedef RedBlackTree<extent_node_t> extent_tree_t; > > +template<typename T> > +int > +CompareAddr(T* a, T* b) 'a' and 'b' don't follow the usual 'aFoo' pattern. (There are multiple examples of this below, too.) ::: memory/build/mozjemalloc.cpp:771 (Diff revision 1) > + { > + return aThis->link; > + } > +}; > + > +struct ArenaRunTreeTrait : public ArenaChunkMapLink It's a bit weird and inconsistent that this trait class and the one just below get their GetTreeNode() method from a superclass, while all the other trait classes define them themselves. Is the use of the superclass necessary? ::: memory/build/mozjemalloc.cpp:776 (Diff revision 1) > +struct ArenaRunTreeTrait : public ArenaChunkMapLink > +{ > + static inline int Compare(arena_chunk_map_t* a, arena_chunk_map_t* b) > + { > + MOZ_ASSERT(a); > + MOZ_ASSERT(b); Most other Compare() functions lack these assertions. Is there a reason for the inconsistency? ::: memory/build/mozjemalloc.cpp:785 (Diff revision 1) > + > +struct ArenaAvailTreeTrait : public ArenaChunkMapLink > +{ > + static inline int Compare(arena_chunk_map_t* a, arena_chunk_map_t* b) > + { > + int ret; Combine the declaration of `ret` with the assignment below. ::: memory/build/mozjemalloc.cpp:2044 (Diff revision 1) > #endif > } > > static void * > -chunk_recycle(extent_tree_t *chunks_szad, extent_tree_t *chunks_ad, size_t size, > - size_t alignment, bool base, bool *zeroed) > +chunk_recycle(RedBlackTree<extent_node_t, ExtentTreeSzTrait> *chunks_szad, > + RedBlackTree<extent_node_t, ExtentTreeTrait> *chunks_ad, You could change these args to aFoo form. Likewise in various other functions. ::: memory/build/mozjemalloc.cpp:2212 (Diff revision 1) > } > > static void > -chunk_record(extent_tree_t *chunks_szad, extent_tree_t *chunks_ad, void *chunk, > - size_t size, ChunkType chunk_type) > +chunk_record(RedBlackTree<extent_node_t, ExtentTreeSzTrait> *chunks_szad, > + RedBlackTree<extent_node_t, ExtentTreeTrait> *chunks_ad, > + void *chunk, size_t size, ChunkType chunk_type) Move the '*' to the left. ::: memory/build/rb.h:668 (Diff revision 1) > -/* > - * The rb_wrap() macro provides a convenient way to wrap functions around the > - * cpp macros. The main benefits of wrapping are that 1) repeated macro > - * expansion can cause code bloat, especially for rb_{insert,remove)(), and > - * 2) type, linkage, comparison functions, etc. need not be specified at every > - * call point. > +/* Tree structure. */ > +template<typename T, typename Trait> > +struct RedBlackTree > +{ > + T* rbt_root; > + T rbt_nil; mRoot and mNil?

Comment on attachment 8912958 [details] Bug 1403444 - Expand rb_new, rb_first, rb_last, rb_next and rb_prev. https://reviewboard.mozilla.org/r/184296/#review189530

Comment on attachment 8912959 [details] Bug 1403444 - Expand rbp_node_new. https://reviewboard.mozilla.org/r/184298/#review189532 ::: memory/build/rb.h:629 (Diff revision 1) > void Init() > { > rbt_root = &rbt_nil; > - rbp_node_new(T, Trait::GetTreeNode, this, &rbt_nil); > + rb_node_field(&rbt_nil, Trait::GetTreeNode).SetLeft(&rbt_nil); > + rb_node_field(&rbt_nil, Trait::GetTreeNode).SetRight(&rbt_nil); > rb_node_field(&rbt_nil, Trait::GetTreeNode).SetColor(NodeColor::Black); So the old code set it to Red and then immediately changed it to Black? +1 for the new code.

Comment on attachment 8912960 [details] Bug 1403444 - Trivially expand rb_node_field. https://reviewboard.mozilla.org/r/184300/#review189536 I admit I didn't check every line of that patch.

Comment on attachment 8912961 [details] Bug 1403444 - Expand rbp_next and rbp_prev. https://reviewboard.mozilla.org/r/184302/#review189538 ::: memory/build/rb.h:576 (Diff revision 1) > T* Next(T* aNode) > { > T* ret; > - rbp_next(T, Trait::GetTreeNode, Trait::Compare, this, (aNode), (ret)); > + if (Trait::GetTreeNode(aNode).Right() != &rbt_nil) { > + rbp_first( > + T, Trait::GetTreeNode, this, Trait::GetTreeNode(aNode).Right(), (ret)); Unnecessary parens around `ret`. ::: memory/build/rb.h:602 (Diff revision 1) > T* Prev(T* aNode) > { > T* ret; > - rbp_prev(T, Trait::GetTreeNode, Trait::Compare, this, (aNode), (ret)); > + if (Trait::GetTreeNode(aNode).Left() != &rbt_nil) { > + rbp_last( > + T, Trait::GetTreeNode, this, Trait::GetTreeNode(aNode).Left(), (ret)); ditto

Comment on attachment 8912962 [details] Bug 1403444 - Expand rb_search and rb_nsearch. https://reviewboard.mozilla.org/r/184304/#review189540

> Come to think of it, keeping integral types would even allow to use bit fields and remove half the manual bit fiddling. If you leave them as integral, please add a comment explaining that they are actually a pointer and a tagged pointer, but they're stored as integers to reduce casting in the methods.

Comment on attachment 8912963 [details] Bug 1403444 - Allow First and Last to take a start node, and use that in Next and Prev. https://reviewboard.mozilla.org/r/184306/#review189542 ::: memory/build/rb.h:517 (Diff revision 1) > } > > - T* First() > + T* First(T* aStart = nullptr) > { > T* ret; > - rbp_first(T, Trait::GetTreeNode, this, rbt_root, (ret)); > + rbp_first(T, Trait::GetTreeNode, this, (aStart ? aStart : rbt_root), (ret)); Unnecessary parens around `ret`. (Probably fixable in an earlier patch.)

Comment on attachment 8912964 [details] Bug 1403444 - Expand rbp_first and rbp_last. https://reviewboard.mozilla.org/r/184308/#review189544

Comment on attachment 8912965 [details] Bug 1403444 - Expand rb_insert. https://reviewboard.mozilla.org/r/184310/#review189548 Holy shit!

Comment on attachment 8912966 [details] Bug 1403444 - Expand rb_remove. https://reviewboard.mozilla.org/r/184312/#review189550 Whoa, that was even worse than rb_insert().

Comment on attachment 8912967 [details] Bug 1403444 - Replace rbp_rotate_left and rbp_rotate_right with private methods. https://reviewboard.mozilla.org/r/184314/#review189552

Comment on attachment 8912968 [details] Bug 1403444 - Replace rbp_lean_left and rbp_lean_right with private methods. https://reviewboard.mozilla.org/r/184316/#review189554

Comment on attachment 8912969 [details] Bug 1403444 - Replace rbp_move_red_left and rbp_move_red_right with private methods. https://reviewboard.mozilla.org/r/184318/#review189556 I sure hope that script of yours doesn't have subtle bugs.

Comment on attachment 8912970 [details] Bug 1403444 - Remove rbp_f_synced. https://reviewboard.mozilla.org/r/184320/#review189558

Comment on attachment 8912971 [details] Bug 1403444 - Replace the rb_foreach_* macros with a range iterator. https://reviewboard.mozilla.org/r/184322/#review189560 ::: memory/build/rb.h:621 (Diff revision 1) > return node; > } > -}; > > -/* > + /* > - * The iterators simulate recursion via an array of pointers that store the > + * The iterator simulate recursion via an array of pointers that store the simulates ::: memory/build/rb.h:645 (Diff revision 1) > - * is: > + * is: > - * > + * > - * (3 * ((SIZEOF_PTR<<3) - (SIZEOF_PTR_2POW+1))) > + * (3 * ((SIZEOF_PTR<<3) - (SIZEOF_PTR_2POW+1))) > - * > + * > - * This works out to a maximum depth of 87 and 180 for 32- and 64-bit > + * This works out to a maximum depth of 87 and 180 for 32- and 64-bit > - * systems, respectively (approximatly 348 and 1440 bytes, respectively). > + * systems, respectively (approximatly 348 and 1440 bytes, respectively). "approximately" is still misspelled :)

Comment on attachment 8912972 [details] Bug 1403444 - Add a helper class to avoid the tree traversal code having to use Trait::GetTreeNode noisily. https://reviewboard.mozilla.org/r/184324/#review189562 Nice. ::: memory/build/rb.h:241 (Diff revision 1) > + > + TreeNode* First(TreeNode* aStart) > + { > + TreeNode* ret; > + for (ret = aStart ? aStart : rbt_root; ret->Left() != &rbt_nil; > + ret = ret->Left()) { I usually put for-loop headers on 3 lines if they don't fit on 1, but not everybody does that.

Comment on attachment 8912973 [details] Bug 1403444 - Rename the RedBlackTree member variables. https://reviewboard.mozilla.org/r/184326/#review189564 Nice.

Thank you for doing this. I'm sure it was a tedious slog, but the codebase will be better for it.

(In reply to Nicholas Nethercote [:njn] from comment #106) > > + T, Trait::GetTreeNode, this, Trait::GetTreeNode(aNode).Right(), (ret)); > > Unnecessary parens around `ret`. > > > + T, Trait::GetTreeNode, this, Trait::GetTreeNode(aNode).Left(), (ret)); > > ditto (In reply to Nicholas Nethercote [:njn] from comment #109) > > + rbp_first(T, Trait::GetTreeNode, this, (aStart ? aStart : rbt_root), (ret)); > > Unnecessary parens around `ret`. (Probably fixable in an earlier patch.) Note those parens all go away in another patch, so... meh. (In reply to Nicholas Nethercote [:njn] from comment #118) > Comment on attachment 8912972 [details] > Bug 1403444 - Add a helper class to avoid the tree traversal code having to > use Trait::GetTreeNode noisily. > > https://reviewboard.mozilla.org/r/184324/#review189562 > > Nice. > > ::: memory/build/rb.h:241 > (Diff revision 1) > > + > > + TreeNode* First(TreeNode* aStart) > > + { > > + TreeNode* ret; > > + for (ret = aStart ? aStart : rbt_root; ret->Left() != &rbt_nil; > > + ret = ret->Left()) { > > I usually put for-loop headers on 3 lines if they don't fit on 1, but not > everybody does that. This is the output from clang-format, fwiw.

(In reply to Nicholas Nethercote [:njn] from comment #102) > > +struct ArenaRunTreeTrait : public ArenaChunkMapLink > > It's a bit weird and inconsistent that this trait class and the one just > below get their GetTreeNode() method from a superclass, while all the other > trait classes define them themselves. Is the use of the superclass necessary? Not necessary, but I like to not repeat myself. The main reason there's a GetTreeNode method in the first place is that in extent_node_t there are two link fields, but all the others only have one, and while they have different name right now because of history, I'm thinking we could make them all use the same name and then that superclass would be shared across all traits except the one for extent_node_t. > ::: memory/build/mozjemalloc.cpp:776 > (Diff revision 1) > > +struct ArenaRunTreeTrait : public ArenaChunkMapLink > > +{ > > + static inline int Compare(arena_chunk_map_t* a, arena_chunk_map_t* b) > > + { > > + MOZ_ASSERT(a); > > + MOZ_ASSERT(b); > > Most other Compare() functions lack these assertions. Is there a reason for > the inconsistency? There's no reason besides "this is what the current code does". I've avoided any behavioral change in this patch queue. > ::: memory/build/mozjemalloc.cpp:2044 > (Diff revision 1) > > #endif > > } > > > > static void * > > -chunk_recycle(extent_tree_t *chunks_szad, extent_tree_t *chunks_ad, size_t size, > > - size_t alignment, bool base, bool *zeroed) > > +chunk_recycle(RedBlackTree<extent_node_t, ExtentTreeSzTrait> *chunks_szad, > > + RedBlackTree<extent_node_t, ExtentTreeTrait> *chunks_ad, > > You could change these args to aFoo form. Likewise in various other > functions. I wanted to avoid adding more noise to this patch. Plus, I think those arguments can actually go away. > ::: memory/build/rb.h:668 > (Diff revision 1) > > -/* > > - * The rb_wrap() macro provides a convenient way to wrap functions around the > > - * cpp macros. The main benefits of wrapping are that 1) repeated macro > > - * expansion can cause code bloat, especially for rb_{insert,remove)(), and > > - * 2) type, linkage, comparison functions, etc. need not be specified at every > > - * call point. > > +/* Tree structure. */ > > +template<typename T, typename Trait> > > +struct RedBlackTree > > +{ > > + T* rbt_root; > > + T rbt_nil; > > mRoot and mNil? Done in the last patch :)