Re: [lmi] Code review: product editor

[Greg Chicares]

On 2007-3-2 15:30 UTC, Evgeniy Tarassov wrote:

[quoting out of order]
> If we carefully review each-other changes then no matter how we do it
> should produce the same results.

Let us take that as our guiding principle and seek the most
efficient way to produce the best results.

> On 3/2/07, Greg Chicares <address@hidden> wrote:
>>
>> Where should these changes be made? Note that I already merged
>> 'product-editor_branch' into HEAD. As of 20070221T0307Z, IIRC,
>> the branch exactly matched HEAD, but I've made many changes in
>> HEAD since then.
> 
> Oh, I should have missed the merge event.

I guess I had sort of assumed that you were seeing all changes.
Some of them are marked with your name in CAPITALS, and many
others represent my attempts to clarify coding guidelines, so
could I ask you to look them over?

One way you could routinely see my changes is to run

  cvs -z3 update -l lmi

periodically; I watch for yours in this similar way:

  /c/lmi/checkouts/current[0]$pushd ../product-editor_branch; cvs -z3 update -r 
product-editor_branch lmi; popd

(I spell that out here so that others in my office can use it
if they like). Another way is to subscribe to email cvs-change
notifications as Vadim does--I myself don't do that, but,
referring to the guiding principle, it doesn't matter which way
you choose.

> All that time I was blindly
> committing into product-editor_branch

I'm not sure anything went wrong here. I think you changed that
branch between 2007-02-11 and 2007-02-15 (while I was away on
vacation), and then not until 2007-03-03 (and I saw those
changes).

> product-editor_branch, which should be considered as
> dead now, right?

Referring to the guiding principle, it's your choice, as far
as I'm concerned: if you use that branch, I'll keep following
it; if you don't use it, I'll ignore it.

>> (A) One way is to send a patch to me by personal email, as a
>> '.tar.bz2' file (inline patches can get mangled, especially
>> if you're using gmail). This may make the most sense for these
>> particular changes right now.
> 
> I will prepare a patch with all the changes made and send it to you asap.

I see that you changed product-editor_branch on 2007-03-03;
does it have exactly the same changes as your patch? I'll
assume it does and work with the files updated in the branch
instead of with the patch.

> Or maybe I should create another branch of HEAD, perform there all the
> changes from product-editor_branch (since 20070221T0307Z - the time of
> merge into HEAD)?

Just to be clear, I think you're saying: capture all the
differences in product-editor_branch between 20070221T0307Z
and yesterday, and apply those differences to a copy of the
current HEAD. Let me know if I've misunderstood that.

But, assuming I've understood it correctly, I'd say 'no' in
this case. I think you're just trying to save my having to
perform a three-way merge, but I've already begun it. I've
heard it said that a three-way merge is more robust than a
two-way merge, so I wanted to try it anyway. And I see some
interesting results, which I'll discuss below.

In case others in my office want to use this technique,
here's a sample command line:

/c/lmi/src/lmi[0]$for z in *; \
  do diff3 --easy-only --merge \
    $z \
    /c/lmi/checkouts/product-editor_branch-20070215/lmi/$z \
    /c/lmi/checkouts/product-editor_branch-20070303/lmi/$z \
  > merged/$z; done

>> (B) Another way would be to think of that branch as "yours"
>> and HEAD as "mine", and work together to resolve differences.
>> I think this is probably the best way, because [...]
> 
> Oh, IMHO it is getting really complicated.

Then we should refer to our guiding principle. Let me reply to
this part later.

>> Bear in mind that we might have
>>   class B            {virtual foo(wxString const&);};
>>   class D : public B {virtual foo(wxString const&);};
>> so, if we change (only) class D first, it won't compile, but
>> we really want to change both B and D.

I think each of us chose not to deal with one such base class,
as discussed below.

> Yes, I'm usually grepping the code for all the appearances of the
> function name to make sure base class version is also updated.

Watch out for macros: e.g.,
  grep MultiDimTable7 *.?pp

Here's the interesting thing that I mentioned above. I had
already tried replacing wxString with std::string a few days
ago, and saved my results in a separate local directory.
Comparing those results to a 'diff3' merge as described
above, I see no overlap: I think we changed completely
different sets of files.

Doesn't that seem extraordinary? For really complex changes
(unlike this one), sometimes I've asked someone else to make
a set of modifications, done the same independently myself,
and then compared the results. Usually there are just a few
significant differences indicating the union of our errors.
In "De Civitate Dei", XVIII:42, Augustine of Hippo speaks of
independent translations of the same book that miraculously
turned out to be identical; I think we've never witnessed
such a miracle in our work, but I've never seen two patches
differ completely, either.

I think I can explain it, though. You steered clear of these
virtuals
  ProductEditorDocument::ReadDocument (wxString const&);
  ProductEditorDocument::WriteDocument(wxString const&);
and their overrides, but we actually can change them because
they aren't defined in wx itself. I steered clear of these
  wxString ValueToString(boost::any const&) const;
  boost::any StringToValue(wxString const&) const;
because they seemed to require changes to a preprocessor-
generated class template.

Anyway, I've committed my pending changes because they don't
overlap yours, and next I'll work through your changes.