Add default copy constructor to SystemError #475
Conversation
|
@vitaut Why does it have a destructor anyway? |
|
So ... are you going to fix this otherwise? Want me to do anything? |
|
Sorry, I hit the wrong button accidentally :D |
|
I'd assume to mark the destructor explicitly noexcept. |
|
Yes, but it could also be defaulted then. |
|
@dschmidt, thanks for the PR. Could you hide default behind a macro similar to @foonathan, unfortunately the dtor is there for a reason. I don't remember the details but they can be dug out from the version control history. |
|
Yes, most probably. Checking. |
|
AFAICT defaulted and deleted functions are supported from the very same compiler versions and thus I used the same version macros as for the deleted functions |
fmt/format.h
Outdated
| # else | ||
| # define FMT_DEFAULTED {} | ||
| # define FMT_DEFAULTED_COPY_CTOR(TypeName) \ | ||
| TypeName(const TypeName&) {} |
There was a problem hiding this comment.
I think the defaulted copy ctor is equivalent to not having a user-defined copy ctor. Having one with an empty body will not work if the class has members. Therefore I suggest defining FMT_DEFAULTED_COPY_CTOR to nothing here:
# define FMT_DEFAULTED_COPY_CTOR(TypeName)FMT_DEFAULTED is not used and can probably be removed.
There was a problem hiding this comment.
Good catch. Will define it to empty.
I added FMT_DEFAULTED analogously to FMT_DELETED because I thought it might come in handy at some point, but I certainly don't insist. Will remove if you prefer.
There was a problem hiding this comment.
I think the defaulted copy ctor is equivalent to not having a user-defined copy ctor.
Note that this is not the case if the class has a user-defined move constructor. If no copy ctor is declared, it won't have a copy constructor. But by writing = default it will have one.
There was a problem hiding this comment.
There are no move semantics in C++98 though...
There was a problem hiding this comment.
Yes, it's just something that should be kept in mind, just in case.
fmt/format.h
Outdated
| @@ -2405,6 +2422,7 @@ class SystemError : public internal::RuntimeError { | |||
| SystemError(int error_code, CStringRef message) { | |||
| init(error_code, message, ArgList()); | |||
| } | |||
| FMT_DEFAULTED_COPY_CTOR(SystemError); | |||
There was a problem hiding this comment.
I suggest moving semicolon to the definition FMT_DEFAULTED_COPY_CTOR to prevent it dangling if the latter is defined to empty.
|
As soon as you're happy with it, I would like to squash the commits. |
|
Commits can be squashed when merging the PR automatically, no need for that. |
|
Any idea what's up with the mingw builds? |
|
Master's also broken so not your fault. |
|
Appveyor updated MinGW - gcc is 5.3.0 now. Typical. |
|
Oops, wrong thread. |
|
So are you happy with this now or do you want me to change anything still? |
|
Looks good to me, thanks for addressing the comments. I'll fix the master and merge this in. |
|
Wooohoo, sounds good. Thanks for the quick feedback cycle! |
|
Merged, thanks! |
This PR fixes
fmt/format.h:2387:3: error: definition of implicit copy constructor for 'SystemError' is deprecated because it has a user-declared destructor [-Werror,-Wdeprecated] ~SystemError() FMT_DTOR_NOEXCEPT;in clang-3.9