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

[Stephen Leake]

On nvm.automate_show_conflicts, I've checked in a partial
implementation of resolving duplicate name conflicts via renaming
(647ae8f26c120d3a0175227173fef119424ec832).

In this thread, I'll ask some questions about coding style and C++
semantics. In another thread, I'll address monotone semantics.

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.


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


In roster_merge.cc
roster_merge_result::resolve_duplicate_name_conflicts, I have a loop
like this:

  std::vector<duplicate_name_conflict>::iterator i = 
duplicate_name_conflicts.begin();
  while (i != duplicate_name_conflicts.end())
    {
      ... handle conflict

      duplicate_name_conflicts.erase(i);

      // no 'i++' needed; erase does that by side effect. 
      // FIXME: is this proper std library usage?

    } // end while

This works on Win32 MinGW, but I suspect it's not portable. I
understand the basic idea of C++ containers, but I have not written
much code with them.

I can't find a reference that defines what 'erase' does to 'i'. I have
"The C++ Programming Language, Second edition" by Stroustrup. I've
ordered the C++ ANSI standard. I poked around on the web, and I found
one reference that says "iterators for items after i are invalidated".

The point of the loop is to handle all of the conflicts and leave the
vector empty, so roster.is_clean() returns true.

So is this code right? 

What would be a better way to do this?


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
  }

Other places in the code use 'symbol' objects instead, for things like
this. Is there a preference? What are the tradeoffs?

I like enum types when the case statements are exhaustive; then the
compiler warns me when I add an enum item. But I think most case
statements for this won't be exhaustive, so symbols might be more
convenient.

-- 
-- Stephe