Re: [lmi] Code review: product editor

[Greg Chicares]

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.

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

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

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.

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
  GetTDBValue(item_data->GetId())
as below, we avoid at least one potential lossy conversion, I
guess. But, looking elsewhere, is it right?
  typedef std::map<int, TDBValue> dict_map;
  class DatabaseDocument {...  dict_map dict_;  ...};
  TDBValue* DatabaseDocument::GetTDBValue(std::size_t index)
    {... return &dict_[index]; ...}
I'd suppose we're invoking this std::map operator:
  T& operator[](const key_type& x);
and I worry about what happens if we have a negative key. Now, a
negative key may be foolish, but I don't think the database code
promises never to use one (perhaps it should?); and maybe
  (int)(unsigned int)(int)(-1)
preserves value, though I'd prefer not to rely on such a thing
[oh...C99 6.3.1.3/3? yick] when I think we can just avoid any
implicit-conversion pitfall by not using unsigned types in the
interface.

  +    bool const is_leaf = item_data && 
!tree.GetChildrenCount(event.GetItem());

I'm confused here. It's a leaf iff it has no children. At first,
I had taken 'item_data &&' as ensuring dereferenceability, but
that variable's not dereferenced here. If we want to assert that
a leaf node must have non-NULL itemdata, then that's what we
should write (though it's far better to establish that as an
upstream invariant and enforce it as few times as is sufficient,
hopefully just once). And what happens if the implicit invariant
here fails--if a leaf has NULL itemdata, or a non-leaf has non-
NULL itemdata? I'd rather avoid having to think about those
possibilities.

  -    table_adapter_->SetTDBValue(document().GetTDBValue(index));
  -
  -    bool is_topic = tree.GetChildrenCount(event.GetItem());
  -
  -    SetLabel(item_data->GetDescription());
  +    std::string item_label;
  +    if(item_data)
  +        {
  +        item_label = item_data->GetDescription();
  +        }
  +    SetLabel(item_label);

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.

  +    TDBValue* selected_tdbvalue = NULL;

Let's hope we don't ever dereference this....

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

       MultiDimGrid& grid = GetGridCtrl();

This looks safe: it can't possibly crash here due to a NULL
pointer, as long as GetGridCtrl() tests that as a precondition
and throws if it's violated. But the purpose of this patch is
to prevent an observed crash, so let's make sure we haven't
left any potentially-unsafe dereference in functions it calls:

  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.