Re: [lmi] Code review: product editor

[Evgeniy Tarassov]

On 3/21/07, Greg Chicares <address@hidden> wrote:
> On 2007-3-21 4:31 UTC, Greg Chicares wrote:
> >
> > Now I'd like to pause to discuss the next two, but let me
> > do that in a separate email.
>
> 20070316T1232Z_database_fix_crash.patch
> (I suppose these comments would also apply to
> 20070316T1236Z_tier_fix_crash.patch)
>
> Pointers fill me with trepidation.
>
> RCS file: /sources/lmi/lmi/database_view.cpp,v
> ...
>        DatabaseTreeItemData* item_data = dynamic_cast<DatabaseTreeItemData*>
>            (tree.GetItemData(event.GetItem())
>            );
>   -    if(!item_data)
>   -        {return;}
>
> The nice thing about this early return was that I could tell
> immediately that this pointer wouldn't be dereferenced later if
> it was NULL here. But maybe the early return was part of the
> problem that needs to be solved.

I removed it to deal with a potential "bug" in wx. I'm not sure that
wxTreeCtrl guarantees that it will never select a root node when it is
hidden. So the previous version of product editor simply bailed out as
soon as it detected that we had to deal with the root node.

The patched version deals with it, because even if it happens somehow
we could simply recover from it just by showing an empty grid.

> Still, I wonder why we maintain a pointer here. I understand that
> wxTreeCtrl::GetItemData() returns a pointer. I understand that
> it's a wxTreeItemData*, and that we need a dynamic_cast to make
> sure it's a DatabaseTreeItemData*. Presumably we'd maintain a
> pointer variable (instead of immediately dereferencing it and
> using a DatabaseTreeItemData&) only if it could be NULL. What I
> wonder is how this could be NULL.
>
> I see two different ways. First,
>   tree.GetItemData(event.GetItem())
> is non-NULL, but
>   dynamic_cast<DatabaseTreeItemData*>(tree.GetItemData(event.GetItem())
> is not. I guess that would be a logic error: we own the treectrl,
> and we wouldn't put itemdata of any other type into it. Let's
> keep this question in the back of our minds: "should we trap
> that potential logic error?".
>
> Second, it could happen that
>   tree.GetItemData(event.GetItem())
> is NULL. But I guess that's under our complete control, in
> DatabaseView::SetupControls() (I'm assuming that any tree node
> can have itemdata). Examining that function, it looks like a node
> has NULL itemdata iff this condition is met:
>   if(name.Idx == name.ParentIdx)
> and apparently that's true only for the root. Suppose we give
> the tree root a non-NULL object for its itemdata; then could we
> give SetupControls() a postcondition like "Every node has non-
> NULL itemdata"?
>
> If we can do that, then there's only one way for 'item_data'
> to be NULL later, and that's a logic error as discussed above,
> so we could just assert that it's not NULL. Untested idea:
>
>   wxTreeItemData* p = tree.GetItemData(event.GetItem());
>   LMI_ASSERT(NULL != p);
>   DatabaseTreeItemData& item_data = dynamic_cast<DatabaseTreeItemData&>(*p);
>
> The dynamic_cast (if I got it right) then throws a standard
> exception in the logic-error case, and existing code should catch
> that and handle it. That seems tidier than
>
>   S* p = whatever();
>   LMI_ASSERT(NULL != p);
>   T* temporary = dynamic_cast<T*>(p);
>   LMI_ASSERT(NULL != temporary);
>   T& variable_we_really_use = *temporary;
>
> and I think it's a good idiom because dynamic_cast<T&> is
> defined that way by C++2003 5.2.7/6-9 . (Tell me if your opinion
> differs, though).

An explicit assert is slightly better from the point of view of
verbosity of the error message -- if we rely on std::runtime_error
exception generated by dynamic_cast then we loose the filename and the
line number where the exception was generated, which could be useful
for error reporting and debugging. Imagine a error popping out to a
user. If it is an exception caught from dynamic_cast, then user does
not have the precious information (filename and line number) to
transmit, so that it could be quickly found and fixed.

> BTW, maybe it should be const:
>   DatabaseTreeItemData const& item_data = ...

Yes, i should have made it const. Sorry.

> Now, why do I belabor this pointer stuff? Let's scan ahead:
>   +    bool const is_leaf = item_data && 
> !tree.GetChildrenCount(event.GetItem());
> Okay, 'item_data &&' guards against NULL. (Is that needed?)
>   +    if(item_data)
>   +        {
>   +        item_label = item_data->GetDescription();
>   +        }
> Okay, 'if(item_data)' guards against NULL.
>   +    if(is_leaf)
>   +        {
>   +        selected_tdbvalue = document().GetTDBValue(item_data->GetId());
>   +        }
> I got stuck there: does 'if(is_leaf)' guard against 'item_data'
> being NULL? I suppose it does, somehow, else you'd have written
> it otherwise. Having studied this much now, I think I can even
> explain why it does (though I couldn't see it at a glance): it's
> because of an implicit postcondition of SetupControls(). That
> postcondition isn't
>   "Every node has non-NULL itemdata of type DatabaseTreeItemData*"
> though: it's
>   "Every node has non-NULL itemdata of type DatabaseTreeItemData*,
>   with the sole exception that the root node has NULL itemdata
>   that you mustn't dereference."
> But some later change might break that postcondition and cause a
> crash.

Oh, actually it is simply because whenever item_data is NULL, is_leaf
is false, because:
+    bool const is_leaf = item_data && !tree.GetChildrenCount(event.GetItem());
It does not rely on any postcondition from SetupControls. But now i
see that i should have written it another -- more clear way.

> Other comments:
>
>   -    std::size_t index = item_data->GetId();
>
> I'm glad to get rid of this. At first, I thought of
>   int wxEvent::GetId() const;
> and worried about an implicit conversion from signed to unsigned.
> Now I look it up and see it's wxTreeItemData::GetId(), which
> returns a wxTreeItemId by const reference, but still I wouldn't
> know whether that's losslessly convertible to std::size_t. By
> passing it directly

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;

Now looking at it -- the class DatabaseTreeItemData should be moved
from databae_view_editor.hpp to database_view.cpp, and it methods
should follow lmi naming convention.

BTW I wanted to ask you before if you want me to rename product_editor
related classes from SomeName to some_name. I should have done that
before :S, but better late than never..

[...]
> I think we could simplify this by making sure every node has
> non-NULL itemdata. Then the original
>   -    SetLabel(item_data->GetDescription());
> should be reliable.

Yes, that should be the best way to clear the code.

>   +    TDBValue* selected_tdbvalue = NULL;
>
> Let's hope we don't ever dereference this....

In DatabaseTableAdapter every time the inner tdbvalue is accessed it
is checked to be non-NULL. I have tried to enforce it by requiring a
non-NULL pointer but it makes the code less understandable since  a
dummy static TDBValue has to be used and additional checks should be
added so that this dummy variable is not modified by mistake.

>   +    if(is_leaf)
>   +        {
>   +        selected_tdbvalue = document().GetTDBValue(item_data->GetId());
>   +        }
>   +    table_adapter_->SetTDBValue(selected_tdbvalue);
>
> When I see '->', I ask: what if it's NULL?
>  - can we prove (maybe with some effort) that it can't be NULL?
>  - can we see *at a glance* that it couldn't *possibly* be NULL?
>  - can we see at a glance that it couldn't possibly *become* NULL
>      here due to some future change elsewhere?
> This variable is
>     boost::shared_ptr<DatabaseTableAdapter> table_adapter_;
> Now, smart pointers prevent memory leaks, and that's good.
> And this one can extend an object's lifetime; is that valuable
> here...or could scoped_ptr have been used instead? If it could,
> then I'd consider storing it as a reference member. That would
> prevent copying, but isn't this already a noncopyable class?
> And it would show us where to enforce the desirable invariant
> that this pointer be always dereferenceable--presumably in the
> ctor.

The table_adapter_ is shared by DatabaseView and a instance of
DatabaseEditorGrid which accepts boost::shared_ptr. We could add an
additional field to DatabaseView which would be a reference to
table_adapter_ but we still need to keep a shared_ptr (somewhere in a
pocket) only to ensure it is not deleted by DatabaseEditorGrid. I'd
add a pair of methods:
DatabaseTableAdapter& DatabaseView::table_adapter() { *table_adapter; }
+ the const version.

[...]
>   inline MultiDimGrid& TreeGridViewBase::GetGridCtrl() const
>   {
>       return *grid_;
>   }
>
> When I see that, I automatically think of changing it this way:
>  - Assert a non-NULL precondition.
>  - Move it from the header to a '.cpp' file. The assertion
>    would require including "assert_lmi.hpp", which in turn
>    includes other headers of its own, so we should resist the
>    urge to add the assertion in the header. I don't imagine
>    there's much benefit to inlining anyway. I tend to avoid
>    writing inline functions in headers these days, because
>    changing the implementation can trigger a lot of recompiling,
>    and I spend a lot of time recompiling already.

Oki. I'll modify the patch and resend a proper version to you asap.

--
ET