[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [lmi] Code review: product editor

From: Evgeniy Tarassov
Subject: Re: [lmi] Code review: product editor
Date: Thu, 29 Mar 2007 17:09:21 +0200

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

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

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]