Re: [Monotone-devel] resolving name conflicts; code style

[Zack Weinberg]

On Fri, May 9, 2008 at 4:54 AM, Stephen Leake
<address@hidden> wrote:
> I've passed app.opts down several layers to merge.cc
> resolve_conflicts. That function then checks for the presence of
> --resolve_conflicts or --resolve_conflicts_file, and calls functions
> that parse the string or file to resolve conflicts.
>
> Is passing app.opts down that low ok? I didn't see a better way to do
> it.

I would suggest passing just the relevant fields of app.opts.  This is both
more maintainable (because it's obvious which options can affect the behavior)
and more convenient if one ever wants to add another call site that specifies
one of the options itself.

Tangentially, please spell multiword options with dashes between words
instead of underscores.  (--resolve-conflicts, --resolve-conflicts-file).  It's
easier to type that way.

> In merge.hh and a couple of other files, I added a copyright line for
> myself. What is the policy on copyright?

I'm not aware of any specific policy.

> In roster_merge.cc, I have this for representing the various
> resolution options:
>
>  enum resolution_t {resolved_none, resolved_content_ws, resolved_rename};
>
>  string image (resolution_t resolution)
>  {
>    switch (resolution)
>      {
>      case resolved_none:
>        return "resolved_none";
>
>      case resolved_content_ws:
>        return "resolved_content_ws";
>
>      case resolved_rename:
>        return "resolved_rename";
>      }
>
>    return ""; // suppress bogus compiler warning
>  }

For this sort of thing I would actually use bare char * to avoid
constructor overhead, but I don't insist on it.  symbol is probably
better from a type system perspective.

For the switch statement, I would recommend a default: clause
that's an invariant failure, like so:

  switch (thing)
  {
    case one: ...; break;
    case two: ...; break;
    case three: ...; break;
    ...
    default:
      I(false);
   }

and don't put anything after the switch.

zw