[libc++] Granularize range_format and format_kind declarations#148876
Merged
philnik777 merged 1 commit intollvm:mainfrom Jul 17, 2025
Merged
[libc++] Granularize range_format and format_kind declarations#148876philnik777 merged 1 commit intollvm:mainfrom
range_format and format_kind declarations#148876philnik777 merged 1 commit intollvm:mainfrom
Conversation
llvmbot
added
the
libc++
Member
|
@llvm/pr-subscribers-libcxx Author: William Tran-Viet (smallp-o-p) Changes
Why?While working on #105430 I ran into an issue implementing [optional.syn] because of a circular Other approaches consideredThe first solution was to simply drop Performance differencesFrom the benchmark runs I've done, the difference is minimal if not very slightly worse(?) fmt_bench_diff.txt Full diff: https://github.com/llvm/llvm-project/pull/148876.diff 1 Files Affected:
diff --git a/libcxx/include/__format/format_context.h b/libcxx/include/__format/format_context.h
index e672ee7ad0581..59bd18f38bf12 100644
--- a/libcxx/include/__format/format_context.h
+++ b/libcxx/include/__format/format_context.h
@@ -21,12 +21,12 @@
#include <__iterator/back_insert_iterator.h>
#include <__iterator/concepts.h>
#include <__memory/addressof.h>
+#include <__memory/construct_at.h>
#include <__utility/move.h>
#include <__variant/monostate.h>
#if _LIBCPP_HAS_LOCALIZATION
# include <__locale>
-# include <optional>
#endif
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -45,6 +45,57 @@ template <class _OutIt, class _CharT>
class basic_format_context;
# if _LIBCPP_HAS_LOCALIZATION
+
+/*
+ * Off-brand std::optional<locale> to avoid having to #include <optional>. This is necessary because <optional> needs
+ * range_format for P3168R2, which would cause an include cycle.
+ */
+
+class __optional_locale {
+private:
+ union {
+ std::locale __loc_;
+ char __null_state_ = '\0';
+ };
+ bool __has_value_ = false;
+
+public:
+ _LIBCPP_HIDE_FROM_ABI __optional_locale() noexcept {}
+ _LIBCPP_HIDE_FROM_ABI __optional_locale(const std::locale& __loc) noexcept : __loc_(__loc), __has_value_(true) {}
+ _LIBCPP_HIDE_FROM_ABI __optional_locale(std::locale&& __loc) noexcept
+ : __loc_(std::move(__loc)), __has_value_(true) {}
+ _LIBCPP_HIDE_FROM_ABI __optional_locale(__optional_locale&& __other_loc) noexcept
+ : __has_value_(__other_loc.__has_value_) {
+ if (__other_loc.__has_value_) {
+ std::__construct_at(&__loc_, std::move(__other_loc.__loc_));
+ }
+ }
+
+ _LIBCPP_HIDE_FROM_ABI ~__optional_locale() {
+ if (__has_value_) {
+ __loc_.~locale();
+ }
+ }
+
+ _LIBCPP_HIDE_FROM_ABI __optional_locale& operator=(std::locale&& __loc) noexcept {
+ if (__has_value_) {
+ __loc_ = std::move(__loc);
+ } else {
+ std::__construct_at(&__loc_, std::move(__loc));
+ __has_value_ = true;
+ }
+ return *this;
+ }
+
+ _LIBCPP_HIDE_FROM_ABI std::locale& __value() noexcept {
+ if (!__has_value_) {
+ __has_value_ = true;
+ std::__construct_at(&__loc_);
+ }
+ return __loc_;
+ }
+};
+
/**
* Helper to create a basic_format_context.
*
@@ -54,7 +105,7 @@ template <class _OutIt, class _CharT>
_LIBCPP_HIDE_FROM_ABI basic_format_context<_OutIt, _CharT>
__format_context_create(_OutIt __out_it,
basic_format_args<basic_format_context<_OutIt, _CharT>> __args,
- optional<std::locale>&& __loc = nullopt) {
+ __optional_locale&& __loc = __optional_locale()) {
return std::basic_format_context(std::move(__out_it), __args, std::move(__loc));
}
# else
@@ -72,8 +123,8 @@ using wformat_context = basic_format_context< back_insert_iterator<__format::__o
template <class _OutIt, class _CharT>
requires output_iterator<_OutIt, const _CharT&>
-class _LIBCPP_PREFERRED_NAME(format_context)
- _LIBCPP_IF_WIDE_CHARACTERS(_LIBCPP_PREFERRED_NAME(wformat_context)) basic_format_context {
+class _LIBCPP_PREFERRED_NAME(format_context) _LIBCPP_IF_WIDE_CHARACTERS(_LIBCPP_PREFERRED_NAME(wformat_context))
+ basic_format_context {
public:
using iterator = _OutIt;
using char_type = _CharT;
@@ -84,11 +135,7 @@ class _LIBCPP_PREFERRED_NAME(format_context)
return __args_.get(__id);
}
# if _LIBCPP_HAS_LOCALIZATION
- _LIBCPP_HIDE_FROM_ABI std::locale locale() {
- if (!__loc_)
- __loc_ = std::locale{};
- return *__loc_;
- }
+ _LIBCPP_HIDE_FROM_ABI std::locale locale() { return __loc_.__value(); }
# endif
_LIBCPP_HIDE_FROM_ABI iterator out() { return std::move(__out_it_); }
_LIBCPP_HIDE_FROM_ABI void advance_to(iterator __it) { __out_it_ = std::move(__it); }
@@ -106,16 +153,16 @@ class _LIBCPP_PREFERRED_NAME(format_context)
// This is done by storing the locale of the constructor in this optional. If
// locale() is called and the optional has no value the value will be created.
// This allows the implementation to lazily create the locale.
- // TODO FMT Validate whether lazy creation is the best solution.
- optional<std::locale> __loc_;
+
+ __optional_locale __loc_;
template <class _OtherOutIt, class _OtherCharT>
friend _LIBCPP_HIDE_FROM_ABI basic_format_context<_OtherOutIt, _OtherCharT> __format_context_create(
- _OtherOutIt, basic_format_args<basic_format_context<_OtherOutIt, _OtherCharT>>, optional<std::locale>&&);
+ _OtherOutIt, basic_format_args<basic_format_context<_OtherOutIt, _OtherCharT>>, __optional_locale&&);
// Note: the Standard doesn't specify the required constructors.
_LIBCPP_HIDE_FROM_ABI explicit basic_format_context(
- _OutIt __out_it, basic_format_args<basic_format_context> __args, optional<std::locale>&& __loc)
+ _OutIt __out_it, basic_format_args<basic_format_context> __args, __optional_locale&& __loc)
: __out_it_(std::move(__out_it)), __args_(__args), __loc_(std::move(__loc)) {}
# else
template <class _OtherOutIt, class _OtherCharT>
|
philnik777
requested changes
Jul 15, 2025
Contributor
philnik777
left a comment
philnik777
left a comment
There was a problem hiding this comment.
I don't think this is a good idea. Could you elaborate on where there is a circular dependency? AFAICT You only need to specialize two variables, which definitely doesn't have to cause circular dependencies.
Member
|
I tend to agree: I would try to break the circular dependency some other way (e.g. a forward declaration header) in order to avoid reimplementing |
Contributor
Author
I absolutely agree that this isn't preferable. I'll keep this open for now and look for another way to get the But here's an example of where it's going wrong: |
Member
|
IMO, what we should do is granularize
Then, from most places where we don't need I think that would break the cycle you encounter above because now |
Contributor
|
If we just need |
smallp-o-p
force-pushed
the
remove_optional_from_format_context
branch
from
c193ffa to
c897f1d
Compare
smallp-o-p
changed the title
std::optional in __format/format_context.h with a re-implementationrange_format and format_kind declarations
ldionne
approved these changes
Jul 15, 2025
philnik777
approved these changes
Jul 17, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/10797 Here is the relevant piece of the build log for the reference |
This was referenced Jul 23, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
- Replaces the use ofstd::optionalin__format/format_context.hwith a simple re-implementation that caters tostd::localeonly.Why?
While working on #105430 I ran into an issue implementing [optional.syn] because of a circular include that looked like the following:
optional -> __format/range_default_formatter.h -> __format/range_formatter.h -> __format/format_context.h -> optional. Onlyformat_kindandrange_formatare needed, and so they looked like candidates to be put into an internal header.### Other approaches consideredThe first solution was to simply dropstd::optionalaltogether, but it resulted in a pretty significant performance drop (numbers to come...) when compared to the 'lazy-load' implementation when running theformat.bench.pass.cppbenchmark.### Performance differencesFrom the benchmark runs I've done, the difference is minimal if not very slightly worse(?)I've attached an example benchmark run offormat.bench.pass.cpp,fmt_baselineis how it's currently done andfmt_candidateare my changes. I will have to do a few more runs to get a better picture.fmt_bench_diff.txtfmt_candidate.txtfmt_baseline.txtstd::range_formatandstd::format_kinddeclarations into a header.