Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add default copy constructor to SystemError #475

Merged
merged 3 commits into from
Feb 25, 2017
Merged

Add default copy constructor to SystemError #475

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f0d7c51
Add default copy constructor to SystemError
dschmidt Feb 23, 2017
2644c2f
Add FMT_DEFAULTED and FMT_DEFAULTED_COPY_CTOR macros
dschmidt Feb 23, 2017
bb6eccf
Incorporate PR feedback
dschmidt Feb 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions fmt/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,23 @@ typedef __int64 intmax_t;
TypeName& operator=(const TypeName&)
#endif

#ifndef FMT_USE_DEFAULTED_FUNCTIONS
# define FMT_USE_DEFAULTED_FUNCTIONS 0
#endif

#ifndef FMT_DEFAULTED_COPY_CTOR
# if FMT_USE_DEFAULTED_FUNCTIONS || FMT_HAS_FEATURE(cxx_defaulted_functions) || \
(FMT_GCC_VERSION >= 404 && FMT_HAS_GXX_CXX11) || FMT_MSC_VER >= 1800
# define FMT_DEFAULTED = default
# define FMT_DEFAULTED_COPY_CTOR(TypeName) \
TypeName(const TypeName&) = default
# else
# define FMT_DEFAULTED {}
# define FMT_DEFAULTED_COPY_CTOR(TypeName) \
TypeName(const TypeName&) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no move semantics in C++98 though...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's just something that should be kept in mind, just in case.

# endif
#endif

#ifndef FMT_USE_USER_DEFINED_LITERALS
// All compilers which support UDLs also support variadic templates. This
// makes the fmt::literals implementation easier. However, an explicit check
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest moving semicolon to the definition FMT_DEFAULTED_COPY_CTOR to prevent it dangling if the latter is defined to empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

FMT_VARIADIC_CTOR(SystemError, init, int, CStringRef)

FMT_API ~SystemError() FMT_DTOR_NOEXCEPT;
Expand Down