[Misc] NFC: Fix -Wdefaulted-function-deleted warnings #20649
Conversation
|
@swift-ci please smoke test |
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
| @@ -38,7 +38,9 @@ struct SingleRawComment { | |||
| SingleRawComment(StringRef RawText, unsigned StartColumn); | |||
|
|
|||
| SingleRawComment(const SingleRawComment &) = default; | |||
| SingleRawComment &operator=(const SingleRawComment &) = default; | |||
|
|
|||
| // 'Range' has no copy assignment operator; therefore: | |||
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
| @@ -524,7 +524,9 @@ struct LLVM_LIBRARY_VISIBILITY ExclusiveBorrowFormalAccess : FormalAccess { | |||
| ExclusiveBorrowFormalAccess & | |||
| operator=(ExclusiveBorrowFormalAccess &&) = default; | |||
|
|
|||
| ExclusiveBorrowFormalAccess() = default; | |||
| // 'swift::Lowering::FormalAccess' has no default constructor; therefore: | |||
| ExclusiveBorrowFormalAccess() = delete; | |||
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. |
| 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.
| @@ -524,7 +524,6 @@ struct LLVM_LIBRARY_VISIBILITY ExclusiveBorrowFormalAccess : FormalAccess { | |||
| 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.
Top-of-tree clang enables
-Wdefaulted-function-deleted, therefore the following changes are needed for warning free builds.