-
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
Formatter specialization for ranges needs to exclude recursive ranges #2968
Comments
Sounds great, a PR is very welcome. Thanks for the detailed writeup. |
Dani-Hub
added a commit
to Dani-Hub/fmt
that referenced
this issue
Jul 6, 2022
…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
Dani-Hub
added a commit
to Dani-Hub/fmt
that referenced
this issue
Jul 6, 2022
…ter specialization for std::filesystem::path
Dani-Hub
added a commit
to Dani-Hub/fmt
that referenced
this issue
Jul 6, 2022
…or the formatter specialization for std::filesystem::path
A pull request has been provided, see #2974. |
vitaut
pushed a commit
that referenced
this issue
Jul 10, 2022
…#2974) * 2954: Add test case * Eliminate extra-test and merge it into existing std-test instead. Add conditionals for filesystem::path testing that does not run into the ambiguity problem. * #2968: Introduce additional compile-time predicate to detect recursive 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 * #2968: Eliminate preprocessor condition that enables the formatter specialization for std::filesystem::path * #2968: Eliminate preprocessor condition that enables the test for the formatter specialization for std::filesystem::path * Use own bool_constant, which is available for all C++ versions * Reintroduce previous workaround but restrict to VS 2015 for now * Comma fix * - Rename is_not_recursive_range to is_nonrecursive_range and add comment 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
mtremer
pushed a commit
to ipfire/ipfire-2.x
that referenced
this issue
Nov 28, 2022
- 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Recently (via commit d2a2320), the existing
formatter
specialization for ranges has been made more robust by introduction of short-circuiting meta-programming logical functions. Even though this is an improvement in the existing code base that cannot rely on C++ language concepts, this is still not enough. This issue is opened to suggest to introduce a non-concept-based compile-time barrier against recursive range types (Such asstd::filesystem::path
orboost::filesystem::path
), corresponding to the specialization suggested in the C++ proposal P2286R8.The reason for this need can be easily explained by observing that on Windows's systems
fmt::is_range
returnstrue
forchar
, see https://godbolt.org/z/88bGdh14r (and on posix systems it returnstrue
forwchar_t
, see https://godbolt.org/z/8e5WEh45Y), that means that while the compiler attempts to find out whether the partial specialization infmt/include/fmt/std.h
Line 65 in c4ee726
fmt/include/fmt/ranges.h
Line 396 in c4ee726
the value for the
Enabler
parameter is evaluated. Iffmt::is_range
returnstrue
, this means that it is guaranteed that also the additional part of the test,fmt/include/fmt/ranges.h
Lines 404 to 407 in c4ee726
is also evaluated and this can lead to an infinite compiler recursion, which can be observed, when attempting to evaluate that part of the expression in an isolated program (For the analysis, we can exclude the influence of the second part involving
detail::has_fallback_formatter
, because this evaluates now by default to a constant value), such as the following one:which results in the following compiler error on Windows systems (Tested for VS 2017, 2019 and VS 2022):
That means that the current mechanism relies on undefined compiler behaviour and we are lucky that any code using the current
formatter
forstd::filesystem::path
fromfmt/std.h
currently still compiles and does what it intends ;-)By introducing an additionally suggested
fmt::details::is_not_recursive_range
trait this problem could be eliminated and would very likely also have the effect that we can enable theformatter
forstd::filesystem::path
again for VS 2017 and below as well.Given this explanation I'm volunteering to make a PULL request to realize this aim. Does this suggested approach sound?
The text was updated successfully, but these errors were encountered: