Re: [lmi] 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:
[...]
>> to enforce the desirable invariant
>> that this pointer be always dereferenceable
[...]
> The table_adapter_ is shared by DatabaseView and a instance of
> DatabaseEditorGrid which accepts boost::shared_ptr.
[...]
> I'd
> add a pair of methods:
> DatabaseTableAdapter& DatabaseView::table_adapter() { *table_adapter; }
> + the const version.

Patch '20070316T1232Z_database_fix_crash.v03.patch' changes
'database_view.hpp' this way:

+    DatabaseTableAdapter& table_adapter() const;
...
-    boost::shared_ptr<DatabaseTableAdapter> table_adapter_;
+    // const ensures we never change the pointer value
+    boost::shared_ptr<DatabaseTableAdapter> const table_adapter_;

Does it seem unusual that this function
    DatabaseTableAdapter& table_adapter() const;
returns a non-const reference? It just looks odd to me.

I think the constness of the smart pointer is irrelevant
to the question I'm asking; it's akin to
    DatabaseTableAdapter* const table_adapter_;
if I understand it correctly, as opposed to this:
    DatabaseTableAdapter const* table_adapter_;
But is there some subtle rationale, e.g., similar to the
reason why this boost::shared_ptr member function:
    T* operator->() const;
is const but returns a non-const pointer?

Anyway, I committed both const and non-const versions:
    DatabaseTableAdapter      & table_adapter()      ;
    DatabaseTableAdapter const& table_adapter() const;
to HEAD; please tell me if that seems like a mistake.
I chose explicitly to assert that the pointer is not null
before dereferencing it. I understand how it can be proved
that it can never be null--today, but things may change.
Years from now, if I read a comment that says this is provable,
I'll wonder whether to believe it; but asserting is believing.

BTW, in addition to replacing
  s/table_adapter_->/table_adapter()./
I also added an assertion here:

 MultiDimGrid* DatabaseView::CreateGridCtrl(wxWindow* panel)
 {
+    LMI_ASSERT(table_adapter_);
     return new(wx) DatabaseEditorGrid(panel, table_adapter_);
 }

just to make assurance doubly sure.