Re: [lmi] Code review: product editor

[Greg Chicares]

On 2007-3-21 17:36 UTC, Evgeniy Tarassov wrote:
>
> Oki. I'll modify the patch and resend a proper version to you asap.

20070316T1232Z_database_fix_crash.v03.patch

This has four major parts:
(1) Rename and move DatabaseTreeItemData from a header to a TU
(2) DatabaseView::SetupControls() changes
(3) Addition of DatabaseView::table_adapter()
(4) DatabaseView::UponTreeSelectionChange() changes
I've tried to approach them in that order, which generally
follows the interdependencies.

(1) Rename and move DatabaseTreeItemData from a header to a TU

(a) I sometimes make mistakes when I try to move code between
files, and then it's hard to figure out where I went wrong:
cvs diff won't be much help. But I can do a local diff of the
'.hpp' against the '.cpp' file with tools like these:
  http://en.wikipedia.org/wiki/Comparison_of_file_comparison_tools
and check everything carefully before committing. That's why, on
20070324T1537Z, I split out this subpart of (1) and committed it
separately, with no other changes commingled.

This is a desirable change. Smaller, less complicated headers
make the system easier to understand. Moving a class into an
unnamed namespace in a '.cpp' file makes it clear that only one
translation unit uses it. And thank you for following through and
removing <wx/treectrl.h> from the header. That simplifies what
Lakos in _Large-Scale C++ Software Design_ calls the physical
design; years ago, when I first applied Lakos's principles, I
achieved a notable improvement in build time (I wish I'd
recorded the measurement).

Comparing to a separate directory where I had applied the patch,
I saw that you had removed the one-line class documentation.
Maybe that was intentional: it may seem obvious. But I decided
to preserve it (and add a period, in the next commit--"no other
changes commingled" on 20070324T1537Z). That's no big deal: you
can't always tell what I'll think is obvious, and it takes
virtually no time for me to do this.

(b) On 20070324T2041Z I applied other impeccable changes from the
patch, noting them in 'ChangeLog'. I was surprised that it even
compiled before making the one really interesting change, but
learned that it really was legal as originally written:

http://groups.google.com/group/comp.std.c++/browse_thread/thread/d2ef22ccb120b652/ba805a689e21d258

which is kind of fascinating. But of course I'm grateful to you
for finding and changing this anyway.

(c) Then I got stuck. I figured I should change the names next,
and that went well until I got to 's/GetId/id/' here:

http://cvs.savannah.gnu.org/viewcvs/lmi/lmi/database_view.cpp?annotate=1.9&sortby=date

On line 188, should I change
    std::size_t index = item_data->GetId();
to
    std::size_t index = item_data->id();
, or leave it alone? That is, should this invoke
  database_tree_item_data::id()
or
  wxTreeItemData::GetId()
? It seems to compile either way, and that confuses me, too: does
wxTreeItemId really have a conversion to std::size_t? The wx
documentation says it's an "opaque data type" and a "handle", so
is it really just a typedef (for now, at least) for std::size_t?
I had hoped that the compiler would accept only one way, so that
I could feel certain I'm not introducing a defect. I don't reach
certainty, either, by comparing to the local directory where I've
applied the whole patch, because that line has changed.

If they're the same thing, then I'd ask why we don't return a
wxTreeItemId instead of std::size_t. But I'm guessing that their
values are different: that the wxTreeItemId is a wx-assigned
identifier, while database_tree_item_data::id_ is lmi's enum
'DatabaseNames'.

Or is it? Digging deeper into this, I see it's a sequential
std::size_t determined by DatabaseView::SetupControls(), but
has to compare equal to an enum DatabaseNames value so that
DatabaseDocument::GetTDBValue(std::size_t) can look it up in
  std::map<int, TDBValue> dict_map
, right? It looks like I botched the type system years ago:
that probably should have been
  std::map<DatabaseNames, TDBValue>
instead. And it looks like static_get_db_names() depends on
the enumerators being sequential and starting at zero. But
'dbnames.hpp' doesn't really document that requirement as it
ought to.

Okay, we'll fix the ancient problems later. For now, please
confirm that I should change line 188 (cited above) to
    std::size_t index = item_data->id();
so that I can proceed with confidence.

Now we come to a crossroads. I really want to make some
changes here. First, I'd like to change the type of
  DatabaseTreeItemData::id_
. I think it ought to be either int (to match 'dict_map') or
(enum) DatabaseNames; do you know which would be better? But I'm
pretty sure it shouldn't be a third type00and especially not
std::size_t, because std::size_t is unsigned. See
  http://svn.haxx.se/dev/archive-2004-10/0074.shtml
for a summary of the reasons to avoid unsigned in the interface,
which goes into more depth than section 16.13 of the lmi coding
standard; or see section 9.2.2 of Lakos's book if you have it.

Second, I knew there had to be a fly in the ointment here:
  http://lists.nongnu.org/archive/html/lmi/2007-03/msg00040.html
It's small, and we can deal with it, but I'd not commit to cvs
before dealing with it. When we do 's/GetId/id/', we wind up
using 'id' in several ways:

 - as a member function;

 - in a parameter-declaration-clause:
     database_tree_item_data(std::size_t id, std::string const& description);

 - as a local variable:
            wxTreeItemId id = tree.AddRoot("");

I found that when I did 's/GetId/id/' and then checked my work by
grepping for 'id'. What should I do?

 (i) Just write a comment about it in HEAD? Then, after I've
 applied all the patches, ask you to diff the branch against
 HEAD and resolve all the comments?

 (ii) Change it myself in HEAD? But I'm not sure what to change
 it to, because I'm not sure exactly what it is. Maybe the
 member names should be 'identifier[_]', if it's really a wx id,
 or something like 'database_key[_]' if that's what it is.

 (iii) Like (ii), but change it in the branch--or send you a
 patch for the patch? That sounds laborious and confusing for
 both of us, and it might also conflict with later changes
 you've already made in the branch.

And I hesitate to do much, lest I change something that a later
patch removes. Until this revised patch, I was looking ahead at
my local copy of the branch (which I believe has all prior
patches) to see whether I shouldn't fret over some temporary
thing that would be changed later. Now, is it the case that
you've updated the branch to reflect this revised patch along
with all others? If that's true, then just let me know, and
I'll update my local copy of the branch from cvs.

BTW, I do appreciate your naming accessors and mutators this
way, without 'get'- and 'set'- prefixes. Especially in more
recent code, I've been trying to follow that convention, which
the standard library uses anyway, e.g.:
  iostate exceptions() const;
  void exceptions(iostate except);
in basic_ios; precision() and width() in ios_base; and str()
in the stringstream classes.

(2) DatabaseView::SetupControls() changes

Shouldn't we use std::auto_ptr only when transfer-of-ownership
semantics are wanted? I don't see the purpose here.

I've tried to use only boost::shared_ptr and boost::scoped_ptr,
and no other smart pointer--in effect treating std::auto_ptr as
a mistake in the standard, like std::vector<bool>, which we
should generally avoid. Maybe that's too extreme a view of
std::auto_ptr, and I'm willing to be shown otherwise, but why
wouldn't boost::scoped_ptr be better here?

Trying to answer my own question, I think I might see a reason:
std::auto_ptr::release() has disown-without-deleting behavior,
which boost::scoped_ptr::reset() does not. Well, now I've learned
a second thing about C++ in this message (the first being that
A::A::A::A thing). Teach me a third: I don't see why you didn't
write release() instead of get() (in both places), and then
omit the later release() call.

Maybe we don't need a smart pointer here, anyway. I think it's
introduced only to give itemdata to the root. Now, AddRoot() is
called only when this condition
        if(name.Idx == name.ParentIdx)
is true. Would it be an error to call AddRoot() twice? I think
we're assuming that (name.Idx == name.ParentIdx) if and only if
(0 == i), so should we assert that? And, if so, then do you think
it would make the code simpler if we call AddRoot() outside the
loop (which I suppose would then start at i = 1)?

Let's not do that with another patch now, though. First we want
to clear all pending patches, while we have this conversation on
the side; I'll refer to this message in HEAD, so that we won't
forget to make any actual change later.

(3) Addition of DatabaseView::table_adapter()

Okay, thanks. I'm sure you won't object if I make a change like

    // the pointer is never NULL
+   LMI_ASSERT(table_adapter_);
    return *table_adapter_;

or add some comment explaining the reason why it can't be null.
You've sometimes added comments to my code that made me think
"of course that's what I meant...oh, maybe it wasn't obvious",
so maybe there's a big advantage in commenting each other's code.

(4) DatabaseView::UponTreeSelectionChange() changes

Could we lose the extra spaces please?
    LMI_ASSERT( item_id.IsOk() );
               ^              ^
    LMI_ASSERT( data );
And in (1), too:
    std::size_t id() const { return id_; }
    std::string const& description() const { return description_; }
Maybe you want to write your own script to catch things like
that? We don't want the 'check_concinnity' makefile target to
grow much slower, and I've probably tuned it to catch only what
I've habitually overlooked myself.

What should I do about this? I don't want to commit to HEAD with
those spaces. I can remove them when committing to HEAD, but that
means that HEAD will diverge increasingly from the branch (as in
other examples above). Is that a problem? If it is, are we sure
we can manage it? Or is it exactly what we want?

At any rate, I'm not going to try committing this part of the
patch yet, because it depends on other parts, and I'm trying to
do this one step at a time, making sure that I'm not introducing
a defect at any step. In the future, I think it'd save me a great
deal of time if I could ask you to make a separate patch for any
major renaming like (1)(c) above. (Does the "tier_fix_crash"
patch do anything like that?)