Re: [lmi] Code review: product editor

[Evgeniy Tarassov]

On 3/26/07, Greg Chicares <address@hidden> wrote:
> (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:

> 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
[...]
> ? It seems to compile either way, and that confuses me, too: does
> wxTreeItemId really have a conversion to std::size_t? The wx
[...]

I'm sorry -- I wrote to you about this isue before. I have named an
accessor for the custom field
 database_tree_item_data::id_
with the same name the base class already have. In this message:
http://lists.nongnu.org/archive/html/lmi/2007-03/msg00035.html
| Ugh, I'm sorry, that is my fault: item_data is of type
| 'DatabaseTreeItemData*' which derives from wxTreeItemData. And by
| coincidence i have named acessor for our own custom field
| DatabaseTreeItemData::id exactly as one of non-virtual methods of
| wxTreeItemData (wxTreeItemData::GetId).
|
| In the code item_data->GetId() refers to
| std::size_t DatabaseTreeItemData::GetId() const;

Sorry for making it complicated. The code meant to use our custom
field, therefore any occurence of GetId() should be changed to id() to
repair the confusing code.

> [...] 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'.

Exactly.

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

Confirmed. Sorry for making you spend time on this issue.

> 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

I wonder why haven't I picked DatabaseNames and used 'unsigned int'
for indexing.

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

Ok. You have mentioned it already on the mailing list. I change the
MDGrid related classes interfaces to use int instead of unsigned int,
but i think you agree that we should do it once we have merged the
current branch back, since the difference between the branch and HEAD
grows really fast.

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

I think that would be the best. Or maybe even to omit this part of the
patch and redo it once the other changes are applied.

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

The 'database_key[_]' is the best because that is 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.

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

Yes, it is exactly what it was used here for.

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

The one feature the boost::scoped_ptr is missing is to be able to be
used as a guard for data. It acts as the owner and there is no way
(AFAIK) to make it release the data without deleting it.

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

Oh, I dont' think I have much to explain to you here. The reason to
use std::auto_ptr is to avoid undeleted object if the call to
tree.AppendItem (or tree.AddRoot) fails with an exception. AFAIK if
you write directly
 tree.AppendItem(..., ptr.release());
then at the time of the function call the auto_ptr does not own the
data anymore, and if the method throws before getting ownership of the
data, then it is lost. Calling ptr.get() and subsequently
ptr.release() only ensures the transfer was successful.

> 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

Yes, it is incorrect to call it twice. An assert will be triggered
(for a debug build) from within WX.

> we're assuming that (name.Idx == name.ParentIdx) if and only if
> (0 == i), so should we assert that? [...]

The code assumes that GetDBNames() array has parents before children
(and therefore the root node has to be the first one in the array). If
someday GetDBNames() violates this then DatabaseView::SetupControls()
will fail with an exception from WX about adding two root nodes.
Because if a child node preceds its parent the
name_to_id[name.ParentIdx] statement will return an empty
wxTreeItemId() which is not valid (wxTreeItemId().IsValid() == false),
and this will trigger and error. But if you feel that this analisys is
too complicated, then an explicit extra-assertion should be added.

> it would make the code simpler if we call AddRoot() outside the
> loop (which I suppose would then start at i = 1)?

The version with AddRoot outside the loop is couple of lines longer
(since we need to store the root node ids -- the id from WX and the id
for lmi). I think it's matter taste. Personally i prefer to have
AddRoot inside the loop because all the secondary operations we need
to do (like storing the created node id 'name_to_id[name.Idx] = id;'
and allocating a new DatabaseTreeItemData -- using std::auto_ptr or
not), are not duplicated. And i think the loop is easier to understand
since the 'if(name.Idx == name.ParentIdx)' is only happen once and
followed directly by AddRoot. IMHO it's all matter of taste.

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

Yes, thank you!