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

[dak]

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.

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