Fix issue 4184: The spec should warn about using copy constructors. #4185
Conversation
jmdavis
requested review from
andralex,
WalterBright,
maxhaton and
mdparker
as code owners
|
Thanks for your pull request, @jmdavis! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
|
spec/struct.dd
Outdated
|
|
||
| For backward compatibility reasons, a `struct` that explicitly defines both | ||
| a copy constructor and a postblit will only use the postblit for implicit | ||
| copying. However, if the postblit is disabled, the copy constructor will be |
There was a problem hiding this comment.
Would mentioning the attribute explicitly be more comprehensible?
i.e.
[…] However, if the postblit is
@disabled, […]
There was a problem hiding this comment.
I don't know why it would be more comprehensible. It would also arguably be wrong if you want to be really technical, because it's possible for a postblit constructor to be disabled for other reasons. The one that comes to mind at the moment would be if one or more of the member variables has had its postblit constructor disabled, and in such a case, the type wouldn't have necessarily declared any postblit or copy constructors (and it would need to declare one to be copyable).
Either way, that part of the message is just copied from what was there already.
|
Ah, apparently, you need a Well, that's been fixed now. |
spec/struct.dd
Outdated
| copying. However, if the postblit is disabled, the copy constructor will be | ||
| used. If a struct defines a copy constructor (user-defined or generated) | ||
| and has fields that define postblits, a deprecation will be issued, | ||
| informing that the postblit will have priority over the copy constructor.) |
There was a problem hiding this comment.
This closing ) has no opening $(P tag. Ditto for the postblit 2nd paragraph.
There was a problem hiding this comment.
As the warnings are each 2 paragraphs, might be nice to wrap them in a $(PANEL.
There was a problem hiding this comment.
Nice catch. I clearly screwed it up when I was rearranging the text and then failed to catch it. I really should just take the time to figure out how to build the documentation locally again to see what it actually looks like. It's been ages since I've done that and didn't want to bother, but I'm clearly making too many mistakes here which would be easily caught if I actually built the documentation.
And I'll have a look at what $(PANEL does. I don't recall seeing that particular macro before.
There was a problem hiding this comment.
Okay. I finally figured out how to build the documentation and see the changes. It looks fine now. I don't know if using PANEL looks better or not, since it's grouping the two paragraphs, but it doesn't affect the count (though the count is kind of odd in a few places already anyway), so it looks a bit funny in that regard. But it probably is better to group them, so the changes now use it.
…ors. Until dlang/dmd#20970 is fixed, we need to be warning against the use of copy constructors rather than warning against the use of postblit constructors. So, I fixed the warning and put both it and the description of what happens when mixing postblit constructors and copy constructors together on both the section for postblit constructors and the one for copy constructors. I suspect that move constructors merit a similar warning, but I'm not sure what their current state is, so this just fixes the warning for postblit constructors and copy constructors.
jmdavis
added
the
72h no objection -> merge
Until dlang/dmd#20970 is fixed, we need to be warning against the use of copy constructors rather than warning against the use of postblit constructors.
So, I fixed the warning and put both it and the description of what happens when mixing postblit constructors and copy constructors together on both the section for postblit constructors and the one for copy constructors.
I suspect that move constructors merit a similar warning, but I'm not sure what their current state is, so this just fixes the warning for postblit constructors and copy constructors.