-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
Annotate fmt::format and fmt::formatted_size as [[nodiscard]] #2612
Conversation
include/fmt/core.h
Outdated
@@ -3071,7 +3071,7 @@ FMT_API auto vformat(string_view fmt, format_args args) -> std::string; | |||
\endrst | |||
*/ | |||
template <typename... T> | |||
FMT_INLINE auto format(format_string<T...> fmt, T&&... args) -> std::string { | |||
[[nodiscard]] FMT_INLINE auto format(format_string<T...> fmt, T&&... args) -> std::string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably needs a macro for ancient compilers which can't ignore unknown attributes.
c95052e
to
26207cf
Compare
include/fmt/core.h
Outdated
#if FMT_HAS_CPP17_ATTRIBUTE(nodiscard) | ||
# define FMT_NODISCARD [[nodiscard]] | ||
#else | ||
# define FMT_NODISCARD | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wrap this in #ifndef FMT_NODISCARD
to allow users override autodetection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply clang-format to your changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that; done.
(Would it be worthwhile to have an automated checker for this?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worthwhile to have an automated checker for this?
Sure, a PR is welcome =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, a PR is welcome =)
Challenge accepted (#2613), but the tools are somewhat crude. Is there a time budget for the CI ?
In another project we grep the output of running git-clang-format, but perhaps I should just contribute/port --dry-run from clang-format to git-clang-format.
26207cf
to
54e4c51
Compare
This prevents accidentally writing fmt::format when fmt::print was intended. Other than running tests, there's not a good use case for discarding the formatted output.
54e4c51
to
4d6afd7
Compare
Merged, thank you! |
This prevents accidentally writing fmt::format when fmt::print was
intended. Other than running tests, there's not a good use case for
discarding the formatted output.