Re: Issue 4156: Define Smob<> constructors. (issue 152370043 by address@hidden)

[nine . fierce . ballads]

On 2014/10/08 08:42:55, dak wrote:
> It complicates the source without _any_ difference in execution as far
> as I can see.

The goal of this change is robustness and maintainability.  From my
perspective, the new version raises fewer questions.

> One side change is to remove copy constructors with an assert(false).
...
> As it stands, an accidental use of a copy will trigger a
> failed assertion with the previous code, and a link error with your
code.  The
> latter is probably somewhat more obscure.

I thought that failing to link would be preferred given that a run time
error might not be discovered until post release.

> I'm not necessarily against that approach (it is more "customary") but
I think
> that there should then be a comment accompanying the declaration of
the copy
> constructor to state clearly that the absence of a copy constructor is
> intentional.

I wrote "// not defined" by the private declarations of the copy
constructors for Context and Music_iterator.  Did I miss one, or would
you like a more elaborate statement of intent?

> In the case of Smob, it is probably sufficient to just do that for
Smob itself
> and not bother with the derived classes like Context and stuff: C++
will then
> not be able to generate a copy constructor anyway.

I started that way, then it became clear that many derived Smobs have
copy constructors.

> lily/context.cc:59: Context::Context (Context const & /* src */)
> contexts may not be copied, ever.  How do you ascertain that?

1. try it
2. fail to link
3. see header where copy constructor is commented "not defined"

> lily/include/book.hh:31: typedef Smob<Book> base_type;
> Any reason for forking this out into a separate typedef?  It's an
additional
> redirection.

To one familiar with this coding pattern, it has the comforting
advantage that when the derived class calls base_type::do_something(),
it is obvious that it is not bypassing part of the inheritance
hierarchy.

It also reduces the number of lines that have to be changed when
refactoring.  Say you wanted to interpose a class between Smob<Thing>
and Thing called SmobWithMore<Thing>.  In Thing.hh, you would change the
following two lines, but you wouldn't have to change calls to
base_type::do_something().

    class Thing : public Smob<Thing> {
        typedef Smob<Thing> base_type;

Since I have found this beneficial, I thought I'd do it here.  If you
don't like it, I'll take it out; please let me know.

> It's worth noting that the protection conses are not currently in use.

> Protection works via scm_gc_protect which works with a hash table.

> Ultimately, one should either use a doubly-linked list instead of a
> protection_cons_ (in order to have faster protection/unprotection than
a hashed
> list will provide) or just not use "new" as the creation mechanism but
rather
> make_smob, returning an SCM.

If you mean that it's worth adding these comments to the code verbatim,
I can do that; however, I can't fully appreciate them because I have no
depth of knowledge of guile yet.

On the subject of "new", defining new and delete operators for Smobs
might be another option.


https://codereview.appspot.com/152370043/diff/1/lily/include/smobs.hh#newcode272
> lily/include/smobs.hh:272: Smob(Smob const &)
> A copy constructor?  What for?  You don't actually call it with a Smob
const &
> but rather with a Super const & unless I am overlooking something.

A copy constructor for constructing a copy.

Smob<Super> is the base class of Super.  A reference to a derived class
can be passed to a function that accepts a reference to its base class.

> And where is
> the point in that if you are not going to read the argument at all
anyway but
> rather use the same as the default initialization?

Expected semantics in the derived class: Want a copy? Call the base
type's copy constructor.  Easily reviewed, no questions asked.

Also, although the default and the copy constructor are the same now, if
you find something you want to do differently in the future, things are
already set up for it.

> Why go to all that effort in the first place?  You just do the same
thing as
> before but in a more complicated manner.

I see it the other way.  Neglecting to initialize members on
construction (or allowing copy-initialization of things that should not
be copied) is a big red flag.  Setting those members when certain
methods are called is a complicated work-around for the
missing/incorrect initialization.

Please consider that in this situation, the reaction of a newbie might
actually carry some weight.  :)  Thanks for taking the time to respond.

https://codereview.appspot.com/152370043/