Skip to content

[libc++][format] Fixes nested concept evaluation. #85548

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

Merged

Conversation

mordante
Copy link
Member

Before the __formattable concept depended on itself in a contrieved example. By using the underlying concept directly the cycle is broken.

Fixes #81590

@mordante mordante requested a review from a team as a code owner March 16, 2024 19:10
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 16, 2024

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

Before the __formattable concept depended on itself in a contrieved example. By using the underlying concept directly the cycle is broken.

Fixes #81590


Full diff: https://github.com/llvm/llvm-project/pull/85548.diff

2 Files Affected:

  • (modified) libcxx/include/__format/format_arg_store.h (+1-2)
  • (added) libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp (+35)
diff --git a/libcxx/include/__format/format_arg_store.h b/libcxx/include/__format/format_arg_store.h
index 066cd369eb8918..23a599e9957599 100644
--- a/libcxx/include/__format/format_arg_store.h
+++ b/libcxx/include/__format/format_arg_store.h
@@ -151,7 +151,7 @@ consteval __arg_t __determine_arg_t() {
 // The overload for not formattable types allows triggering the static
 // assertion below.
 template <class _Context, class _Tp>
-  requires(!__formattable<_Tp, typename _Context::char_type>)
+  requires(!__formattable_with<_Tp, _Context>)
 consteval __arg_t __determine_arg_t() {
   return __arg_t::__none;
 }
@@ -165,7 +165,6 @@ _LIBCPP_HIDE_FROM_ABI basic_format_arg<_Context> __create_format_arg(_Tp& __valu
   using _Dp               = remove_const_t<_Tp>;
   constexpr __arg_t __arg = __determine_arg_t<_Context, _Dp>();
   static_assert(__arg != __arg_t::__none, "the supplied type is not formattable");
-
   static_assert(__formattable_with<_Tp, _Context>);
 
   // Not all types can be used to directly initialize the
diff --git a/libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp b/libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp
new file mode 100644
index 00000000000000..0969d3bc21dfad
--- /dev/null
+++ b/libcxx/test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp
@@ -0,0 +1,35 @@
+//===----------------------------------------------------------------------===//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: GCC-ALWAYS_INLINE-FIXME
+
+// The sample code is based on the bug report
+// https://github.com/llvm/llvm-project/issues/81590
+//
+// Tests whether this formatter does not fail to compile due to nested concept
+// evaluation.
+
+#include <format>
+#include <variant>
+
+struct X : std::variant<X*> {
+  X* p = nullptr;
+  constexpr const std::variant<X*>& decay() const noexcept { return *this; }
+};
+
+template <>
+struct std::formatter<X, char> : std::formatter<std::string, char> {
+  static constexpr auto format(const X& x, auto ctx) {
+    if (!x.p)
+      return ctx.out();
+    auto m = [&](const X* t) { return std::format_to(ctx.out(), "{}", *t); };
+    return std::visit(m, x.decay());
+  }
+};
+
+void bug_81590() { (void)std::format("{}", X{}); }

@mordante mordante force-pushed the review/fixes_formatter_concept_dependency branch 2 times, most recently from 683f837 to a80c9a0 Compare March 18, 2024 13:36
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

LGTM with minor comment.

Before the __formattable concept depended on itself in a contrieved
example. By using the underlying concept directly the cycle is broken.

Fixes llvm#81590
@mordante mordante force-pushed the review/fixes_formatter_concept_dependency branch from a80c9a0 to e3049c4 Compare March 20, 2024 06:17
@mordante mordante merged commit 11dd881 into llvm:main Mar 20, 2024
@mordante mordante deleted the review/fixes_formatter_concept_dependency branch March 20, 2024 08:46
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
Before the __formattable concept depended on itself in a contrieved
example. By using the underlying concept directly the cycle is broken.

Fixes llvm#81590
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libc++ formatter concept depends on itself
3 participants