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

[dak]

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.


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