Re: [lmi] Code review: product editor

[Greg Chicares]

On 2007-3-2 10:59 UTC, Evgeniy Tarassov wrote:
> On 3/1/07, Greg Chicares <address@hidden> wrote:
>> (1) Class template MultiDimTableTypeTraits duplicates lmi's
>> function template value_cast(). I would much rather use the lmi
> [...]
>> (2) I'd prefer to use standard types instead of wx types where
>> possible. IIRC, wx container types are just synonyms for STL
>> containers anyway, but the string types are not substitutable.
> 
> Sorry for that. MultiDimTableTypeTraits removed and value_cast used
> everywhere, where a conversion is needed.

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.

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

(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 we're going
to be working together on this for a while. The big advantage
is that each of us has complete liberty to commit anything we
want to, without delay (in different places). The challenge
it poses is that we both have to work to review each other's
changes promptly.

But that challenge is inherent. For either of us to skip code
review in the hope of going faster is a mistake IMO. Careful
peer review certainly takes more time up front, but it'll save
time in the long run. In the past, we have sometimes taken
forever to review your work, I know--after all, you originally
wrote this code in 2005--but that enormous delay was due to
obstacles on this side of the ocean that we have now removed.

Maybe we should take approach (A) in the short term, in order
to get the system stabilized sooner so that others can start
testing it, and then switch to (B). I'll leave that decision
completely to you.

If you can think of another way that would produce better
results, then I'm glad to listen. To me, "better" means a more
robust system, because what we really want is a rock-solid
implementation that either of us can maintain. "Better" also
means that we aren't wasting time needlessly. What "better"
must not mean is that we sacrifice quality in the hope of
finishing sooner than is really possible, because then we'll
spend the next few months fixing defects we had overlooked in
our haste.

> wxString is replaced by
> std::string where possible. There are however two occurences of
> wxString in constructors left untouched for only consistency with wx.
> For example rounding_view_editor.hpp
>    RoundingButtons
>        (wxWindow* parent
>        ,wxWindowID id
>        ,const wxPoint& pos = wxDefaultPosition
>        ,const wxSize& size = wxDefaultSize
>        ,long style = 0
>        ,const wxString& name = wxPanelNameStr
>        );
> Do you think it also should be changed to use lmi own types? It left
> untouched only for consistency with its wx base class.

Based on the experiments I've done so far, I think the rule
should be to use std::string unless that makes it impossible
to compile at all. Taking that as a postulate, I suspect that
we can deduce this theorem: use std::string everywhere except
in virtual overrides of a wx library function that uses class
wxString in its signature. We may discover edge cases that
weaken that theorem or require us to reformulate it; e.g., I
haven't considered default arguments, which may be tricky...
but the compiler will let us know.

Here's a case where I believe we must use wxString:
  class CensusDocument : public wxDocument {
    virtual bool OnOpenDocument(wxString const& filename);
but I'm not sure why we can't change the signature of the
RoundingButtons ctor--I just tried that, and it seemed okay.

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.

> A little question which is not related directly to the above.

Let me answer that in a separate email.