-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Exclude recursive ranges from the formatter specialization for ranges #2974
Exclude recursive ranges from the formatter specialization for ranges #2974
Conversation
… conditionals for filesystem::path testing that does not run into the ambiguity problem.
…cursive ranges and reject them in formatter specialization for ranges. In addition, introduce additional wrapper traits for the individual logical operands of the complete range constraints
…ter specialization for std::filesystem::path
…or the formatter specialization for std::filesystem::path
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.
Thanks for the PR.
include/fmt/ranges.h
Outdated
@@ -390,22 +390,36 @@ using range_formatter_type = conditional_t< | |||
template <typename R> | |||
using maybe_const_range = | |||
conditional_t<has_const_begin_end<R>::value, const R, R>; | |||
|
|||
template <typename R> | |||
struct is_not_recursive_range : bool_constant< |
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.
is_not_recursive_range -> is_nonrecursive_range
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.
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.
Renamed as suggested.
include/fmt/ranges.h
Outdated
template <typename R, typename Char> | ||
struct is_formattable_delayed : is_formattable< | ||
detail::uncvref_type<detail::maybe_const_range<R>>, Char> { | ||
}; | ||
|
||
template <typename R, typename Char> | ||
struct has_fallback_formatter_delayed : detail::has_fallback_formatter< | ||
detail::uncvref_type<detail::maybe_const_range<R>>, Char> { | ||
}; |
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.
What are these for? I'd suggest keeping them inlined as before.
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.
The whole point of this P/R is to ensure that the individual parts of the condition are evaluated in short-circuit manner. In C++ without language concepts this is only possible by defining type predicates that get only evaluated if the corresponding predicate template class definition becomes instantiated. Due to std::conjunction
and friends we therefore need individual class templates. The trailing delayed
is intended to emphasize their job.
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.
Could you clarify this in a comment?
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.
I'd suggest either adding a generic thing to delay instantiation, like so:
template <template <class...> class F, typename... Args> struct delayed : F<Args...> { };
to be used as:
disjunction<
delayed<is_formattable, detail::uncvref_type<detail::maybe_const_range<R>>, Char>,
delayed<detail::has_fallback_formatter, detail::uncvref_type<detail::maybe_const_range<R>>, Char>
>
or, if we're going to manually do our own thing, do all of it together:
template <class T, class Char>
struct delayed_fallback_formattable
: disjunction<
is_formattable<T, Char>,
detail::has_fallback_formatter<T, Char>>
{ };
to be used as:
conjunction<
A,
B,
delayed_fallback_formattable<detail::uncvref_type<detail::maybe_const_range<R>>, Char>
>
Put differently - instead of writing two bespoke delayed traits, write either one generic delayer or one bespoke delayed trait.
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.
This is a good suggestion but note that has_fallback_formatter
will soon go away so I suggest keeping two bespoke traits. Keeping both will simplify removing has_fallback_formatter_delayed
later.
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.
Oh. Ok that makes sense then. When you say soon, how soon?
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.
I merged now has_fallback_formatter_delayed into is_formattable_delayed and got rid of the former.
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.
how soon?
In the next major release about a year from now.
include/fmt/ranges.h
Outdated
|
||
template <typename R> | ||
struct is_not_recursive_range : bool_constant< | ||
!std::is_same<detail::uncvref_type<R>, R>::value> {}; |
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.
Contrary to the name of this trait it only checks recursiveness but not whether R is a range. I suggest adding is_range check here.
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.
Again, we want to prevent instantiation if the is_recursive predicate unless the is_range predicate has evaluated to true. See above. We could rename it to something different, such as is_not_recursive
, if you prefer and add a comment that says that we assume here that is_range is evaluated to true.
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.
Yes, please add a comment.
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.
Comment added, also relating to P2286.
…ent that explains it being depending on is_range being true - Merge has_fallback_formatter_delayed into is_formattable_delayed and add comment that explains it being depending on is_not_recursive_range being true - Replace disjunction in formatter specialization by has_fallback_formatter_delayed - Get rid of unneeded detail:: prefixes within namespace detail
Merged, thanks! |
- Update from version 9.0.0 to 9.1.0 - Update of rootfile - Changelog 9.1.0 - 2022-08-27 * ``fmt::formatted_size`` now works at compile time `#3026 <https://github.com/fmtlib/fmt/pull/3026>`_ For example (`godbolt <https://godbolt.org/z/1MW5rMdf8>`__): .. code:: c++ #include <fmt/compile.h> int main() { using namespace fmt::literals; constexpr size_t n = fmt::formatted_size("{}"_cf, 42); fmt::print("{}\n", n); // prints 2 } * Fixed handling of invalid UTF-8 `#3038 <https://github.com/fmtlib/fmt/pull/3038>`_, `#3044 <https://github.com/fmtlib/fmt/pull/3044>`_, `#3056 <https://github.com/fmtlib/fmt/pull/3056>`_ * Improved Unicode support in ``ostream`` overloads of ``print`` `#2994 <https://github.com/fmtlib/fmt/pull/2994>`_, `#3001 <https://github.com/fmtlib/fmt/pull/3001>`_, `#3025 <https://github.com/fmtlib/fmt/pull/3025>`_ * Fixed handling of the sign specifier in localized formatting on systems with 32-bit ``wchar_t`` `#3041 <https://github.com/fmtlib/fmt/issues/3041>`_). * Added support for wide streams to ``fmt::streamed`` `#2994 <https://github.com/fmtlib/fmt/pull/2994>`_ * Added the ``n`` specifier that disables the output of delimiters when formatting ranges `#2981 <https://github.com/fmtlib/fmt/pull/2981>`_, `#2983 <https://github.com/fmtlib/fmt/pull/2983>`_ For example (`godbolt <https://godbolt.org/z/roKqGdj8c>`__): .. code:: c++ #include <fmt/ranges.h> #include <vector> int main() { auto v = std::vector{1, 2, 3}; fmt::print("{:n}\n", v); // prints 1, 2, 3 } * Worked around problematic ``std::string_view`` constructors introduced in C++23 `#3030 <https://github.com/fmtlib/fmt/issues/3030>`_, `#3050 <https://github.com/fmtlib/fmt/issues/3050>`_ * Improve handling (exclusion) of recursive ranges `#2968 <https://github.com/fmtlib/fmt/issues/2968>`_, `#2974 <https://github.com/fmtlib/fmt/pull/2974>`_ * Improved error reporting in format string compilation `#3055 <https://github.com/fmtlib/fmt/issues/3055>`_ * Improved the implementation of `Dragonbox <https://github.com/jk-jeon/dragonbox>`_, the algorithm used for the default floating-point formatting `#2984 <https://github.com/fmtlib/fmt/pull/2984>`_ * Fixed issues with floating-point formatting on exotic platforms. * Improved the implementation of chrono formatting `#3010 <https://github.com/fmtlib/fmt/pull/3010>`_ * Improved documentation `#2966 <https://github.com/fmtlib/fmt/pull/2966>`_, `#3009 <https://github.com/fmtlib/fmt/pull/3009>`_, `#3020 <https://github.com/fmtlib/fmt/issues/3020>`_, `#3037 <https://github.com/fmtlib/fmt/pull/3037>`_ * Improved build configuration `#2991 <https://github.com/fmtlib/fmt/pull/2991>`_, `#2995 <https://github.com/fmtlib/fmt/pull/2995>`_, `#3004 <https://github.com/fmtlib/fmt/issues/3004>`_, `#3007 <https://github.com/fmtlib/fmt/pull/3007>`_, `#3040 <https://github.com/fmtlib/fmt/pull/3040>`_ * Fixed various warnings and compilation issues `#2969 <https://github.com/fmtlib/fmt/issues/2969>`_, `#2971 <https://github.com/fmtlib/fmt/pull/2971>`_, `#2975 <https://github.com/fmtlib/fmt/issues/2975>`_, `#2982 <https://github.com/fmtlib/fmt/pull/2982>`_, `#2985 <https://github.com/fmtlib/fmt/pull/2985>`_, `#2988 <https://github.com/fmtlib/fmt/issues/2988>`_, `#3000 <https://github.com/fmtlib/fmt/issues/3000>`_, `#3006 <https://github.com/fmtlib/fmt/issues/3006>`_, `#3014 <https://github.com/fmtlib/fmt/issues/3014>`_, `#3015 <https://github.com/fmtlib/fmt/issues/3015>`_, `#3021 <https://github.com/fmtlib/fmt/pull/3021>`_, `#3023 <https://github.com/fmtlib/fmt/issues/3023>`_, `#3024 <https://github.com/fmtlib/fmt/pull/3024>`_, `#3029 <https://github.com/fmtlib/fmt/pull/3029>`_, `#3043 <https://github.com/fmtlib/fmt/pull/3043>`_, `#3052 <https://github.com/fmtlib/fmt/issues/3052>`_, `#3053 <https://github.com/fmtlib/fmt/pull/3053>`_, `#3054 <https://github.com/fmtlib/fmt/pull/3054>`_ Signed-off-by: Adolf Belka <adolf.belka@ipfire.org> Reviewed-by: Michael Tremer <michael.tremer@ipfire.org>
No description provided.