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/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
