Re: [Monotone-devel] some observations about roster node IDs

[Zack Weinberg]

On 6/28/07, Nathaniel Smith <address@hidden> wrote:
> On Thu, Jun 28, 2007 at 09:35:51PM -0500, Timothy Brownawell wrote:
> > On Thu, 2007-06-28 at 15:07 -0700, Zack Weinberg wrote:
[...]
> It's really one of those "C++ sucks, it doesn't have
> enough features" things, you can either use dynamic_cast stuff and
> have proper (static) type-checking on field accesses, or use enum
> stuff and have proper (static) warnings if you have left something out
> of your switch statement.  Or use boost's discriminated union type,
> which is theoretically perfect and pure torture on the programmer.
>
> Wrapper classes (or checked accessors generally) might be a least-bad
> solution.

I was considering two options.  In one, everything is as it is now
except that we put a type tag in node_t, check it in
is_dir_t/is_file_t, and use static_cast in downcast_to_*.  The node_t
constructor is protected and the file_t/dir_t constructors set the
type tags.  I think this is essentially your "enum stuff" approach.

The other, more radical option involves splitting the nodes map, so
there's one for dirs and one for files.  get_node has two out
parameters, one a dir_t & and one a file_t &, and returns a boolean
telling you which it filled in.  This also has the advantage that
there are a bunch of places in roster.cc (get_node itself not least)
which only want to know about directories.  (You'd have to either have
two maps for children, or do as you suggested below and store node_ids
in the ->children map.)

I really don't know which is better.

> > The lookup just uses the hashtable, so it really shouldn't show up until
> > everything else is sped up a lot. The map is only there to provide
> > ordered iteration (for eg roster_merge.cc, which does parallel iteration
> > on rosters).
>
> Possibly those few places that need sorted items should just call
> sort?

Possibly.  There aren't many of them.  I worry a little about memory overhead.

zw