----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1141/#review2544 -----------------------------------------------------------
Nice! Thanks. My only comment is that it still seems strange to have lookupHandle() in the public interface, particularly so now that it's not used anywhere in the x86 TLB code. [Aside: another reason to have the x86 TLB code folded in to the same patch... so we can more easily see how the trie code is supposed to be used at the same time we're reviewing it! ;-) not trying to start an argument, just couldn't resist making the point...] I can see where if someone wanted to use the trie but didn't want to bother tracking all the handles themselves, then you might think you'd need this. However, the only thing a user can do with a handle is call remove on it, so I'd advocate for making an overloaded remove(Key key) method like this: Handle h = lookupHandle(key); if (h) remove(h); You could even return a bool if you think the caller will ever care whether something actually got removed or not. Then lookupHandle() could be made protected, and you have a nice clean public interface that's basically just insert/lookup/remove, and the whole Handle thing is just an optimization to avoid an extra lookup by allowing users to call remove(Handle) instead of remove(Key). And people reading your code don't have to wonder why you've bothered to factor lookupHandle() out of lookup() even though no one else ever calls it... src/base/trie.hh <http://reviews.gem5.org/r/1141/#comment2956> I think you mean Handle here (and elsewhere in this comment). - Steve Reinhardt On April 14, 2012, 3:17 a.m., Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1141/ > ----------------------------------------------------------- > > (Updated April 14, 2012, 3:17 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 8943:d726ff94da31 > --------------------------- > sim: A trie data structure specifically to speed up paging lookups. > > This change adds a trie data structure which stores an arbitrary pointer type > based on an address and a number of relevant bits. Then lookups can be done > against the trie where the tree is traversed and the first legitimate match > found is returned. > > > Diffs > ----- > > src/base/trie.hh PRE-CREATION > src/unittest/SConscript 5534a564f6a0 > src/unittest/trietest.cc PRE-CREATION > > Diff: http://reviews.gem5.org/r/1141/diff/ > > > Testing > ------- > > > Thanks, > > Gabe Black > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
