Issue 4156: Define Smob<> constructors.

Define Smob<> constructors.
https://code.google.com/p/lilypond/issues/detail?id=4156

[Dan Eble, 2014-10-08 04:40:36]

Note that I haven't searched for kinds of derived Smob that are still getting
compiler-generated constructors.  My approach was to clean up Smob<T> and fix
resulting warnings.  The rest shouldn't be any worse off now than they were
before.

[lemzwerg, 2014-10-08 04:57:41]

LGTM, from visual inspection only :-)

https://codereview.appspot.com/152370043/diff/1/lily/include/book.hh
File lily/include/book.hh (right):

https://codereview.appspot.com/152370043/diff/1/lily/include/book.hh#newcode31
lily/include/book.hh:31: typedef Smob<Book> base_type;
I would insert an empty line before the `public' keyword...

[dak, 2014-10-08 08:42:55]

Neither the Google issue nor the Rietveld review contain any rationale for that
change.  It complicates the source without _any_ difference in execution as far
as I can see.

One side change is to remove copy constructors with an assert(false). 
Personally, I am annoyed at C++ not allowing to declare
class MyType {
  Mytype (const Mytype&) = 0;
}
and be done with it.  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'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.

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.

https://codereview.appspot.com/152370043/diff/1/lily/context.cc
File lily/context.cc (left):

https://codereview.appspot.com/152370043/diff/1/lily/context.cc#oldcode59
lily/context.cc:59: Context::Context (Context const & /* src */)
contexts may not be copied, ever.  How do you ascertain that?

https://codereview.appspot.com/152370043/diff/1/lily/include/book.hh
File lily/include/book.hh (right):

https://codereview.appspot.com/152370043/diff/1/lily/include/book.hh#newcode31
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.

https://codereview.appspot.com/152370043/diff/1/lily/include/smobs.hh
File lily/include/smobs.hh (right):

https://codereview.appspot.com/152370043/diff/1/lily/include/smobs.hh#newcode269
lily/include/smobs.hh:269: protection_cons_ (SCM_EOL)
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.

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

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

[Dan Eble, 2014-10-08 13:17:39]

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.

[dak, 2014-10-08 13:54:07]

On 2014/10/08 13:17:39, Dan Eble wrote:
> 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.

You do several different things in this patch.  For some, "raises fewer
questions" is sort of subjective.  But for the title-giving change, "raises
fewer questions" is very much inaccurate.

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

As I said: that's the part of the patch that I can get behind.

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

"Not defined" is quite definitely not a statement of intent.  Nor is it of
purpose.  It is of fact.  And it is quite confusing since it is immediately
adjacent to a declaration.

It's more like
// declared, do not define!  Prevents default copy constructor.

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

Well, but those should not be using a Smob copy constructor but rather the Smob
default constructor because a Smob is not copyable.  You need to have a new one.
 You write some sort of semi-copy-constructor that does not actually copy. 
That's not an improvement in clarity.

I'll see whether I can wrap up a patch for that.

[Dan Eble, 2014-10-08 23:48:16]

On 2014/10/08 13:54:07, dak wrote:
> "Not defined" is quite definitely not a statement of intent.  Nor is it of
> purpose.  It is of fact.  And it is quite confusing since it is immediately
> adjacent to a declaration.
> 
> It's more like
> // declared, do not define!  Prevents default copy constructor.

I assumed one level of familiarity with this convention, and you assumed
another.  To me, the presence of a comment signals intent.  (Not arguing, just
saying.)

Maybe you would prefer something like this:
http://www.boost.org/doc/libs/1_56_0/libs/core/doc/html/core/noncopyable.html

> Well, but those should not be using a Smob copy constructor but rather the
Smob
> default constructor because a Smob is not copyable.

I agree to the extent that copying is not part of a Smob's public interface,
however I respectfully disagree with your conclusion.  Generally speaking, it is
contradictory to copy a derived object without copying the members it inherits. 
In the current implementation, the set of inherited members worth copying is
empty, but that should not be reflected in the code of the derived class.

> I'll see whether I can wrap up a patch for that.

Please clarify what that means for my patch.  Is there anything to keep, or
should I abandon it?  Thanks.

[dak, 2014-10-09 05:13:48]

On 2014/10/08 23:48:16, Dan Eble wrote:
> On 2014/10/08 13:54:07, dak wrote:
> > "Not defined" is quite definitely not a statement of intent.  Nor is it of
> > purpose.  It is of fact.  And it is quite confusing since it is immediately
> > adjacent to a declaration.
> > 
> > It's more like
> > // declared, do not define!  Prevents default copy constructor.
> 
> I assumed one level of familiarity with this convention, and you assumed
> another.  To me, the presence of a comment signals intent.  (Not arguing, just
> saying.)
> 
> Maybe you would prefer something like this:
> http://www.boost.org/doc/libs/1_56_0/libs/core/doc/html/core/noncopyable.html

That makes some sense in a library framework like Boost with its own
conventions.  But basically anything you have to look up is a reading
obstacle in an application.  The comment should be definite enough
that one does not bother looking.

> > Well, but those should not be using a Smob copy constructor but
> > rather the Smob
> > default constructor because a Smob is not copyable.
> 
> I agree to the extent that copying is not part of a Smob's public interface,
> however I respectfully disagree with your conclusion.  Generally speaking, it
is
> contradictory to copy a derived object without copying the members
> it inherits.

When the base class is solely responsible for managing per-copy
semantics, it is not "contradictory" to _not_ transfer it to a copy,
neither conceptually nor factually.

The copy constructor calls smobify_self () and that is called on a
smob not yet in use.  Initializing it from the source does not make
sense.

> In the current implementation, the set of inherited members worth
> copying is empty, but that should not be reflected in the code of
> the derived class.

Smob is a base class used for memory management.  Before templates, it
was common to use a base class for creating linked lists and other
data structures.  In that case, I'd consider it also contradictory to
copy the per-copy data by default just to overwrite it afterwards.
And with things like separate garbage collection threads doing
conservative stack and heap scanning (which we have in GUILEv2) we
don't _want_ an initialization with legit-looking material.

Your code did not actually copy any data, but one had to dig up the
respective definitions to make sure.

> > I'll see whether I can wrap up a patch for that.
> 
> Please clarify what that means for my patch.  Is there anything to keep, or
> should I abandon it?  Thanks.

Rietveld does not allow patches from more than one author in one
Rietveld review.  So that means if you want to work incrementally from
a proposed patch, you have the option of either making comments to the
patch, or of uploading a version of the patch in a review of your own.
The last upload "wins" in the Google code issue in that it is tested
and commented on by default.

In this particular case I uploaded a "competing" patch since
accommodating my propositions was a considerable amount of work and
I care enough about it that I would see the approaches compared on
discussed on their respective merits rather than have them ride on the
"but somebody would need to do it" theme.

For reviewing, that's worse than working on the same Rietveld review
since you cannot compare versions.  Incidentally, do you use git cl
for uploading?  Your reviews are remarkable in that they do not allow
comparing/viewing the various versions of your uploads.  That should
not usually happen so it would be interesting to know what causes this
situation.

So for single changes, it makes more sense to comment on the last
uploaded variant of a patch.  For sweeping changes or competing
proposals, it may make more sense to upload to a Rietveld issue you
own yourself.

When you don't do anything, usually the last uploaded patch eventually
makes it into countdown and code.  Unless the discussion around it
seems to indicate a lack of workable consensus among developers.

So in a nutshell: you don't _need_ to do anything more if you consider
the last proposed state acceptable.

[Dan Eble, 2014-10-11 17:53:13]

On 2014/10/09 05:13:48, dak wrote:
> Incidentally, do you use git cl
> for uploading?  Your reviews are remarkable in that they do not allow
> comparing/viewing the various versions of your uploads.  That should
> not usually happen so it would be interesting to know what causes this
> situation.

I have been using "git cl upload origin/master" with my branch checked out. 
What is responsible for determining the differences between uploaded patch sets?

[dak, 2014-10-11 18:52:57]

On 2014/10/11 17:53:13, Dan Eble wrote:
> On 2014/10/09 05:13:48, dak wrote:
> > Incidentally, do you use git cl
> > for uploading?  Your reviews are remarkable in that they do not allow
> > comparing/viewing the various versions of your uploads.  That should
> > not usually happen so it would be interesting to know what causes this
> > situation.
> 
> I have been using "git cl upload origin/master" with my branch checked out. 
> What is responsible for determining the differences between uploaded patch
sets?

I'm not sure.  One thing is likely that "my branch" has a certain continuity in
being related to the same issue (git cl records some info in .git/config
per-branch).  What I usually do is to have my branches associated with an
upstream branch.

git branch -b issuexxx origin

when creating the branch would do that, as would an explicit

git branch --set-upstream=origin

I have no idea whether one of the two is definitely involved.  But keeping the
same branch around is likely important.  Renaming it is fine: I do that
frequently and it keeps the relation to the issue around just fine.

[Dan Eble, 2014-10-18 16:38:14]

On 2014/10/11 18:52:57, dak wrote:
> What I usually do is to have my branches associated with an
> upstream branch.
> 
> git branch -b issuexxx origin

"branch" is a thinko for "checkout" right?

> I have no idea whether one of the two is definitely involved.  But keeping the
> same branch around is likely important.  Renaming it is fine: I do that
> frequently and it keeps the relation to the issue around just fine.

Thanks for the suggestions.  I tried associating my branch with a reference
branch, and I no longer had to provide the reference branch name to git cl
upload, but my reviews still did not show the deltas between patch sets.  Maybe
there is still something out of whack from when I switched google accounts.  I'm
going to start from scratch with LilyDev 3.  If that doesn't work, at least I
will have a newer system to show for it; and if further debugging is required,
it will benefit the latest LilyDev.

[dak, 2014-10-18 16:42:22]

On 2014/10/18 16:38:14, Dan Eble wrote:
> On 2014/10/11 18:52:57, dak wrote:
> > What I usually do is to have my branches associated with an
> > upstream branch.
> > 
> > git branch -b issuexxx origin
> 
> "branch" is a thinko for "checkout" right?

Uh, yes.

Sorry.