Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Constexpr formatted_size #3026

Merged
merged 11 commits into from
Aug 10, 2022
Merged

Constexpr formatted_size #3026

merged 11 commits into from
Aug 10, 2022

Conversation

marksantaniello
Copy link
Contributor

Minimal changes to make fmt::formatted_size constexpr in C++20. Adjust the existing unit test accordingly.

vitaut
vitaut reviewed Aug 8, 2022
View reviewed changes
Copy link
Contributor

@vitaut vitaut left a 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! One minor comment inline, otherwise LGTM.

test/compile-test.cc Outdated
Comment on lines 231 to 234
static FMT_CONSTEXPR20 size_t s1 = fmt::formatted_size(FMT_COMPILE("{0}"), 42);
EXPECT_EQ(2, s1);
static FMT_CONSTEXPR20 size_t s2 = fmt::formatted_size(FMT_COMPILE("{0:<4.2f}"), 42.0);
EXPECT_EQ(5, s2);
Copy link
Contributor

Choose a reason for hiding this comment

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

static seems unnecessary, please remove.

@vitaut
Copy link
Contributor

vitaut commented Aug 8, 2022

CI is failing: https://github.com/fmtlib/fmt/runs/7734651761?check_suite_focus=true

@marksantaniello
Copy link
Contributor Author

Seems Clang-11 has some issue with memcpy not being constexpr. Trying to repro but for me clang 11 actually crashes...

@marksantaniello
Copy link
Contributor Author

OK got a repro. I was missing that I needed to use libc++. Will investigate.

@marksantaniello
Copy link
Contributor Author

Seems the problem is here:

https://github.com/fmtlib/fmt/blob/master/include/fmt/format.h#L295-L305

I think Clang 11 supports -std=c++20, but the included libc++ doesn't have __cpp_lib_bit_cast, so we end up trying to use memcpy in a constexpr function.

@marksantaniello
Copy link
Contributor Author

Maybe I can work around this by using __builtin_bit_cast

@phprus
Copy link
Contributor

phprus commented Aug 9, 2022 •
edited
Loading

For C++20 constexpr fmt needs a bit_cast.
bit_cast requires (https://en.cppreference.com/w/cpp/compiler_support):
GCC libstdc++: 11 (_GLIBCXX_RELEASE >= 11, based on: __builtin_bit_cast)
Clang libc++: 14 (_LIBCPP_VERSION >= 14000, based on: __builtin_bit_cast)
MSVC STL: 19.27

I suggest checking these conditions or __builtin_bit_cast when defining FMT_CONSTEXPR20.
cc @vitaut

@marksantaniello, You can enable Github Actions for your fmt fork

@marksantaniello
Copy link
Contributor Author

@phprus

You can enable Github Actions for your fmt fork

Are you saying that there's a way to get all the same test workflows running on my fork?

@phprus
Copy link
Contributor

phprus commented Aug 9, 2022

@marksantaniello
Yes.

@marksantaniello
Copy link
Contributor Author

Thanks. I'll have to figure do that if I make PRs in the future. The test matrix is pretty large.

Anyway, it looks like the current version using __builtin_bit_cast is passing the checks.

@phprus
Copy link
Contributor

phprus commented Aug 9, 2022

Current test matrix does not check:
gcc-10 + C++20 (defined FMT_CONSTEXPR20, but not support __builtin_bit_cast)
Clang libc++ < 14 + C++20

@marksantaniello
Copy link
Contributor Author

Ah, OK. Maybe I should figure out the workflows now then. I enabled "Actions" but I'm not sure how to clone the checks from here. And then I guess I'd need to add at least two more?

@phprus
Copy link
Contributor

phprus commented Aug 9, 2022

Strategies are defined in the .github/workflows/linux.yml file:

fmt/.github/workflows/linux.yml

Lines 11 to 61 in 8bd02e9

strategy:
matrix:
cxx: [g++-4.8, g++-10, clang++-9]
build_type: [Debug, Release]
std: [11]
os: [ubuntu-18.04]
include:
- cxx: g++-4.8
install: sudo apt install g++-4.8
os: ubuntu-18.04
- cxx: g++-8
build_type: Debug
std: 14
install: sudo apt install g++-8
os: ubuntu-18.04
- cxx: g++-8
build_type: Debug
std: 17
install: sudo apt install g++-8
os: ubuntu-18.04
- cxx: g++-10
build_type: Debug
std: 17
os: ubuntu-18.04
- cxx: g++-11
build_type: Debug
std: 20
os: ubuntu-20.04
install: sudo apt install g++-11
- cxx: clang++-8
build_type: Debug
std: 17
cxxflags: -stdlib=libc++
os: ubuntu-18.04
install: sudo apt install clang-8 libc++-8-dev libc++abi-8-dev
- cxx: clang++-9
build_type: Debug
fuzz: -DFMT_FUZZ=ON -DFMT_FUZZ_LINKMAIN=ON
std: 17
os: ubuntu-18.04
- cxx: clang++-11
build_type: Debug
std: 20
os: ubuntu-20.04
- cxx: clang++-11
build_type: Debug
std: 20
cxxflags: -stdlib=libc++
os: ubuntu-20.04
install: sudo apt install libc++-11-dev libc++abi-11-dev
- shared: -DBUILD_SHARED_LIBS=ON

I think for the test you need to add:
gcc-9,gcc-10 + C++20

@vitaut
Copy link
Contributor

vitaut commented Aug 9, 2022

FMT_CONSTEXPR20 should definitely not be changed. I suggest making the test conditional on whether __cpp_lib_bit_cast is defined, removing the bit_cast changes and new CI configs (we already have way too many).
Then everything that used to work will continue to work and if bit_cast is constexpr and there is C++20 constexpr support then formatted_size will usable in constexpr.

vitaut
vitaut requested changes Aug 9, 2022
View reviewed changes
test/compile-test.cc Outdated
Comment on lines 230 to 241
TEST(compile_test, formatted_size) {
EXPECT_EQ(2, fmt::formatted_size(FMT_COMPILE("{0}"), 42));
EXPECT_EQ(5, fmt::formatted_size(FMT_COMPILE("{0:<4.2f}"), 42.0));
#ifdef __cpp_lib_bit_cast
FMT_CONSTEXPR20
#endif
size_t s1 = fmt::formatted_size(FMT_COMPILE("{0}"), 42);
EXPECT_EQ(2, s1);
#ifdef __cpp_lib_bit_cast
FMT_CONSTEXPR20
#endif
size_t s2 = fmt::formatted_size(FMT_COMPILE("{0:<4.2f}"), 42.0);
EXPECT_EQ(5, s2);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's conditionally compile the test case itself because the whole purpose of it is to test constexpr formatted_size. Also formatted_size -> contexpr_formatted_size to make it clear what we are testing here.

#ifdef __cpp_lib_bit_cast
TEST(compile_test, constexpr_formatted_size) {
  FMT_CONSTEXPR20 size_t s1 = fmt::formatted_size(FMT_COMPILE("{0}"), 42);
  EXPECT_EQ(2, s1);
  FMT_CONSTEXPR20 size_t s2 = fmt::formatted_size(FMT_COMPILE("{0:<4.2f}"), 42.0);
  EXPECT_EQ(5, s2);
}
#endif

.github/workflows/linux.yml Outdated
Comment on lines 31 to 43
- cxx: g++-9
build_type: Debug
std: 20
install: sudo apt install g++-9
os: ubuntu-18.04
- cxx: g++-10
build_type: Debug
std: 17
os: ubuntu-18.04
- cxx: g++-10
build_type: Debug
std: 20
os: ubuntu-18.04
Copy link
Contributor

Choose a reason for hiding this comment

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

As commented earlier, let's not add new CI configs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, yes -- I'm just crap at git. Will try again.

@vitaut
Copy link
Contributor

vitaut commented Aug 10, 2022

Looks good but there are some merge conflicts, please rebase.

@vitaut vitaut merged commit fd93b63 into fmtlib:master Aug 10, 2022
@vitaut
Copy link
Contributor

vitaut commented Aug 10, 2022

Thanks!

dimztimz added a commit to dimztimz/fmt that referenced this pull request Aug 10, 2022
@dimztimz
Revert "Constexpr formatted_size (fmtlib#3026)"
cb04e04 
This reverts commit fd93b63.
mtremer referenced this pull request in ipfire/ipfire-2.x Nov 28, 2022
@pmu-ipf
fmt: Update to version 9.1.0
90d43e5 
- 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 <[email protected]>
Reviewed-by: Michael Tremer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitaut vitaut vitaut approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marksantaniello @vitaut @phprus