Re: Provide some more smob functionality and use it (issue 135240043 by address@hidden)

[dak]

On 2014/09/01 02:58:09, dak wrote:

https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh


https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh#newcode248
> lily/include/smobs.hh:248: class Smob: public Smob_base<Super> {
> On 2014/09/01 01:54:44, eble wrote:
> > Are the compiler-generated copy constructor and assignment operator
going to
> do
> > the right thing?  If so, a comment to that effect would assure us
that you
> > considered them.  If not, they should either be implemented (if
useful) or
> > declared but not defined (if not useful).

> Indeed, a Smob (as opposed to Simple_smob and the Smob1... forms)
should only be
> handled via SCM and/or protected pointers.  private copy constructors
would make
> sense.  Will do and recheck.

> In my defense, it's not like the old code did that either.

Ok, I am pulling the "old code did not do this either" defense for good
since I get
In file included from ./include/listener.hh:64:0,
                 from ./include/translator-group.hh:23,
                 from audio-element-info.cc:22:
./include/smobs.hh: In copy constructor
'Translator_group::Translator_group(const Translator_group&)':
./include/smobs.hh:239:3: error: 'Smob<Super>::Smob(const Smob<Super>&)
[with Super = Translator_group]' is private
   Smob (const Smob<Super> &);
   ^
In file included from audio-element-info.cc:22:0:
./include/translator-group.hh:50:7: error: within this context
 class Translator_group : public Smob<Translator_group>
       ^
In file included from ./include/translator.hh:25:0,
                 from ./include/translator-group.hh:24,
                 from audio-element-info.cc:22:
./include/translator-group.hh: In member function 'virtual
Translator_group* Translator_group::clone() const':
../flower/include/virtual-methods.hh:45:27: note: synthesized method
'Translator_group::Translator_group(const Translator_group&)' first
required here
     return new name (*this);                    \
                           ^
./include/translator-group.hh:70:3: note: in expansion of macro
'VIRTUAL_COPY_CONSTRUCTOR'
   VIRTUAL_COPY_CONSTRUCTOR (Translator_group, Translator_group);
   ^

So clearly making the copy constructor private is not a no-brainer.  The
change would actually be not in this issue but issue 4082 which is
basically a conversion issue from macros to templates.  Since squeezing
in privatization of constructors/assignment turns out to be tricky in
itself, I'm not going to put it in the same issue.  A separate commit,
issue and testing seems called for and I don't see a reason to stop the
horses for this one if the trivial course does not work.  Do you want to
try your hand on this one once issue4082 is through?

https://codereview.appspot.com/135240043/