[Misc] NFC: Fix -Wdefaulted-function-deleted warnings #20649
Conversation
|
@swift-ci please smoke test |
jrose-apple
left a comment
jrose-apple
left a comment
There was a problem hiding this comment.
I feel like many of these are indicative of other bugs, and should not necessarily be silenced in this way. But maybe that just means tweaking the comments to include "FIXME" or something.
I don't think we should ever have a case where something has a move constructor but not a move assignment operator except if there's a very good reason. If it's just const members, we should un-constify them unless it's semantically important that all the const members are initialized together.
include/swift/AST/RawComment.h
Outdated
There was a problem hiding this comment.
That seems like a bug, actually. CharSourceRange should be a trivial type. Or is this about the const?
There was a problem hiding this comment.
'const' does seem to be the problem.
lib/SILGen/LValue.h
Outdated
There was a problem hiding this comment.
I think we'd probably just leave this out.
davezarzycki
force-pushed
the
fix_Wdefaulted-function-deleted-warnings
branch
from
8ce832e to
bf7f91b
Compare
|
Hi @jrose-apple – I was worried about this devolving into something like "const correctness" whack-a-mole, but it turns out to be not that bad. Please consider the updated patch. |
|
@swift-ci please smoke test |
|
@davezarzycki thanks for fixing this, I had gotten half way through the same set of changes and then got worried how deep the rabbit hole was. |
compnerd
left a comment
compnerd
left a comment
There was a problem hiding this comment.
I had tried to use nullability attributes for the ProjectionTree to provide the same guarantees, but that got out of hand pretty quick.
| CharSourceRange() {} | ||
| CharSourceRange() = default; | ||
|
|
||
| CharSourceRange &operator=(const CharSourceRange &) = default; |
There was a problem hiding this comment.
Rule of Zero says that we should not declare this explicitly. If you think it's important, we should follow the Rule of Five instead.
There was a problem hiding this comment.
CharSourceRange() {} is effectively CharSourceRange() = default, right? If so, then yes, I can flush out the rule-of-five.
There was a problem hiding this comment.
Rule of Five doesn't include the default constructor anyway, so just deleting this newest line is fine. The reason to keep it would be to see if we regress, I guess, but then we should definitely include all five = default special members.
| TreeScopedHashTableVal(const TreeScopedHashTableVal &) = delete; | ||
| TreeScopedHashTableVal(TreeScopedHashTableVal &&) = delete; | ||
| TreeScopedHashTableVal &operator=(const TreeScopedHashTableVal &) = delete; | ||
| TreeScopedHashTableVal &operator=(TreeScopedHashTableVal &&) = delete; |
There was a problem hiding this comment.
Rule of Five says it's worth mentioning the destructor here too.
| ExclusiveBorrowFormalAccess & | ||
| operator=(ExclusiveBorrowFormalAccess &&) = default; | ||
|
|
||
| ExclusiveBorrowFormalAccess() = default; |
There was a problem hiding this comment.
The superclass doesn't have a default constructor, so the subclass can't either.
3 participants
Top-of-tree clang enables
-Wdefaulted-function-deleted, therefore the following changes are needed for warning free builds.