[lmi] Safe and consistent dereferencing and casting [Was: Code review: product editor]

[Greg Chicares]

On 2007-3-21 17:36 UTC, Evgeniy Tarassov wrote:
> On 3/21/07, Greg Chicares <address@hidden> wrote:
>>
>> 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.

That's a good point. What you suggest is okay with me for the moment.
Yet this is a problem I'd rather solve consistently, and only once,
in a way that addresses the issue you've raised.

I look in HEAD for places where I've already used or suggested a
cast to reference:

grep 'dynamic_cast.*& *>' *.?pp
any_member_test.cpp:    dynamic_cast<base_datum&>(datum).base_function();
census_view.cpp:        return dynamic_cast<CensusDocument&>(*GetDocument());
database_view.cpp:      return dynamic_cast<DatabaseDocument&>(*GetDocument());
ihs_rnddata.cpp:        return dynamic_cast<rounding_rules&>(*this);
illustration_view.cpp:  return 
dynamic_cast<IllustrationDocument&>(*GetDocument());
inputillus.cpp:         dynamic_cast<InputParms&>(*this) = z;
multidimgrid_any.cpp:   //   
dynamic_cast<MultiDimAxisAnyChoice&>(choice_control)->PopulateChoiceList();
multidimgrid_tools.hpp: //   MultiDimAxisAnyChoice& choice = 
dynamic_cast<MultiDimAxisAnyChoice&>(choice_control);
policy_view.cpp:        return dynamic_cast<PolicyDocument&>(*GetDocument());
rounding_view.cpp:      return dynamic_cast<RoundingDocument&>(*GetDocument());
tier_view.cpp:          return dynamic_cast<TierDocument&>(*GetDocument());
view_ex.cpp:            return 
dynamic_cast<DocManagerEx&>(*GetDocumentManager());
view_ex.cpp:            return dynamic_cast<wxFrame&>(*GetFrame());

These need work anyway--maybe sooner rather than later.
Here's the last one:

  wxFrame& ViewEx::FrameWindow() const
  {
      return dynamic_cast<wxFrame&>(*GetFrame());
  }

The wx-2.8.0 documentation for GetFrame() says
  "Gets the frame associated with the view (if any)."
so wx doesn't guarantee it's not NULL, so I think I should change
the body at least this much:

  wxWindow* f = GetFrame();
  LMI_ASSERT(f);
  return dynamic_cast<wxFrame&>(*f);

And you make a case for something like this:

  wxWindow* w = GetFrame();
  LMI_ASSERT(w);
  wxFrame* f = dynamic_cast<wxFrame&>(*w);
  LMI_ASSERT(f);
  return *f;

in order to get __FILE__ and __LINE__ if the cast fails. That
also tells us whether the pointer was null originally, or the
type was wrong.

Copying a one-line idiom is okay, but copying a five-line idiom
ten times doesn't sound right. Here's a different idea:

-    return dynamic_cast<wxFrame&>(*GetFrame());
+    return safely_dereference_as<wxFrame>(GetFrame());

Please tell me your opinion, because I'm thinking of implementing
that soon. It seems clear to read. The function template can deduce
its argument type. Using RTTI for the two types in the error message
would make it easy to find the offending code: the only case that
RTTI wouldn't suffice to disambiguate is

multidimgrid_any.cpp:   //   
dynamic_cast<MultiDimAxisAnyChoice&>(choice_control)->PopulateChoiceList();
multidimgrid_tools.hpp: //   MultiDimAxisAnyChoice& choice = 
dynamic_cast<MultiDimAxisAnyChoice&>(choice_control);

and there I was wondering whether static type checking would be
possible, e.g.:

-    Windows axis_choice_wins_;
+    std::vector<MultiDimAxisAnyChoice*> axis_choice_wins_;