Re: [lmi] Code review: product editor

[Evgeniy Tarassov]

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

Why should it return a const reference and not a mutable? We are
holding a pointer to table_adapter.

> 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_;

Oh, i think it is more like:
   DatabaseTableAdapter *const table_adapter_;

We hold data as a pointer to non-const structure, but the pointer
itself is const. This ensures that once the pointer is assigned the
value (in the class constructor) it never changes it. The behavior is
the same as for a reference. And the reason to use a boost::shared_ptr
instead of a reference is the data-object lifetime, which is not
necessarily limited to the parent-object.

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

Yes, the pair of non-const and const methods is more usual -- this
looks like the class is offering a setter/getter for its member
variable. IMHO we dont have any obvious reason not to use
const-version-returning-non-const-reference.

> 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

Hmm. The existence of the 'const' keyword in
   boost::shared_ptr<DatabaseTableAdapter> const table_adapter_;
proves that the pointer is never NULL once the parent-object is
successfully constructed. IMHO that extra assertion asserts nothing --
it only makes code more easy to revise (since the question 'could it
be possibly NULL?' will _not_ popup if there is an explicit
assertion).

> Years from now, if I read a comment that says this is provable,
> I'll wonder whether to believe it; but asserting is believing.

Maybe this magic-const deserves a comment -- do you want me to add it
near this line?
   boost::shared_ptr<DatabaseTableAdapter> const table_adapter_;

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

:)

Seriously -- I think a debug time assertion would be ideal for such a
situation. It does not affect performance in release build, but does
the job in debug build, so a developer does not think is it really
necessary to add it here.

Oh, and please correct me if I have missed it -- lmi does not have one, right?

--
With best wishes,
Evgeniy Tarassov