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

[Stephen Leake]

"Zack Weinberg" <address@hidden> writes:

> 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.

That makes sense. And now I see it is in HACKING; I guess I need to
read that again :).

There doesn't seem to be a way to pass an options set. There are four
relevant fields; resolve_conflicts_given,  resolve_conflicts,
resolve_conflicts_file_given, resolve_conflicts_file. That's unwieldy
for a parameter list; it would be nice to be able to pass the set.

In this case, I guess I should parse the options in the top level
command function, to check for syntax errors, and pass the resulting
structure down.

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

Ok. That's not in HACKING; I'll add it.

>> 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.  

Ah, that makes sense. And it compiles and runs without other changes.

> 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);
>    }

Right, if the cases don't cover all the possible values.

> and don't put anything after the switch.

This I don't understand; what's wrong with more processing after the
switch? It won't get executed if you hit the default case, but that's
true of all invariant checks.

--
-- Stephe