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

[dak]

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.

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