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

C++17: std::char_traits<>::{compare,length} is constexpr. #2246

Merged
merged 1 commit into from
Apr 23, 2021
Merged

C++17: std::char_traits<>::{compare,length} is constexpr. #2246

merged 1 commit into from
Apr 23, 2021

Conversation

phprus
Copy link
Contributor

@phprus phprus commented Apr 20, 2021

Discussion: #2243 (comment)

@phprus phprus mentioned this pull request Apr 20, 2021
Merged
alexezeder
alexezeder suggested changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@alexezeder alexezeder left a comment

Choose a reason for hiding this comment

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

Just a few suggestions 😉

include/fmt/format.h Outdated
@@ -2149,7 +2149,10 @@ FMT_CONSTEXPR OutputIt write(OutputIt out, Char value) {
}

template <typename Char, typename OutputIt>
FMT_CONSTEXPR OutputIt write(OutputIt out, const Char* value) {
#if FMT_HAS_CONSTEXPR_CHAR_TRAITS // C++17's char_traits::length() is constexpr.
FMT_CONSTEXPR
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to use FMT_CONSTEXPR macro here, just constexpr would be enough.

include/fmt/core.h Outdated
Comment on lines 471 to 478
#if FMT_HAS_CONSTEXPR_CHAR_TRAITS // C++17's char_traits::compare() is constexpr.
constexpr
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to make this function constexpr in this PR, I think.
This can be done as needed in the following PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is for consistency across all uses of constexpr std::char_traits.

Copy link
Contributor

Choose a reason for hiding this comment

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

All I'm just saying is that this function is non-constexpr currently, unlike basic_string_view constructor or write(const Char* value), so there is no need to make it constexpr in this PR.

include/fmt/color.h Outdated
@@ -432,7 +432,10 @@ template <typename Char> struct ansi_color_escape {
FMT_CONSTEXPR operator const Char*() const FMT_NOEXCEPT { return buffer; }

FMT_CONSTEXPR const Char* begin() const FMT_NOEXCEPT { return buffer; }
FMT_CONSTEXPR const Char* end() const FMT_NOEXCEPT {
#if FMT_HAS_CONSTEXPR_CHAR_TRAITS // C++17's char_traits::length() is constexpr.
FMT_CONSTEXPR
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to use FMT_CONSTEXPR macro here, just constexpr would be enough.

include/fmt/core.h Outdated
Comment on lines 46 to 60
#if __cplusplus >= 201703L
# ifndef __GLIBCXX__
# define FMT_HAS_CONSTEXPR_CHAR_TRAITS 1
# elif defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE >= 7
# define FMT_HAS_CONSTEXPR_CHAR_TRAITS 1
# endif
#endif
#ifndef FMT_HAS_CONSTEXPR_CHAR_TRAITS
# define FMT_HAS_CONSTEXPR_CHAR_TRAITS 0
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest you comment on this check because it's not a really straightforward one 🙂

@phprus phprus force-pushed the constexpr-char-traits branch from c67230c to 8327f37 Compare April 21, 2021 16:02
@phprus phprus force-pushed the constexpr-char-traits branch from 8327f37 to 2e049b4 Compare April 22, 2021 18:03
vitaut
vitaut requested changes Apr 23, 2021
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. In general looks good but let's replace FMT_HAS_CONSTEXPR_CHAR_TRAITS with FMT_CONSTEXPR_CHAR_TRAITS that expands to constexpr or nothing depending on availability of constexpr char traits. This will make usage cleaner which is important because we plan to use this macro in multiple places.

@vitaut
Copy link
Contributor

vitaut commented Apr 23, 2021

Actually looking at the latest diff, you already made the suggested change =). GitHub confusingly shows the old code in Conversation.

@vitaut vitaut merged commit 128f007 into fmtlib:master Apr 23, 2021
@alexezeder
Copy link
Contributor

alexezeder commented Apr 26, 2021 •
edited
Loading

It looks like there is a problem that no one can ever expect: https://godbolt.org/z/Ps7q8dGME
TLDR: __cplusplus is 030c1fH = 199711 on MSVC even with /std:c++17 option, because it's MSVC I guess...
🤦 🤦 🤦

@phprus
Copy link
Contributor Author

phprus commented Apr 27, 2021

Oh!!!
My big mistake...

I can make pull request tonight.

Fix:

#if __cplusplus >= 201703L
#  ifndef __GLIBCXX__
#    define FMT_CONSTEXPR_CHAR_TRAITS constexpr
#  elif defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE >= 7
#    define FMT_CONSTEXPR_CHAR_TRAITS constexpr
#  endif
#endif

to:

#if defined(_MSC_VER)
// VS 2017 15.7+ (https://docs.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance?view=msvc-150)
#  if _MSVC_LANG >= 201703L && _MSC_VER >= 1914
#    define FMT_CONSTEXPR_CHAR_TRAITS constexpr
#  endif
#elif __cplusplus >= 201703L
#  if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 4000 
// libc++4.0+ (https://libcxx.llvm.org/docs/Cxx1zStatus.html)
#    define FMT_CONSTEXPR_CHAR_TRAITS constexpr
#  elif defined(__GLIBCXX__) && defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE >= 7
#    define FMT_CONSTEXPR_CHAR_TRAITS constexpr
#  endif
#endif

New comment:

// Check if constexpr std::char_traits<>::compare,length is supported.
// libstdc++: GCC 7 and newer and __cplusplus >= 201703L
// MSVC: VS 2017 15.7 and newer and /std:c++17
// libc++: 4.0 and newer and if __cplusplus >= 201703L
// NOTE: FMT_GCC_VERSION  - is not libstdc++ version.
//       _GLIBCXX_RELEASE - is present in GCC 7 libstdc++ and newer.

@phprus
Copy link
Contributor Author

phprus commented Apr 27, 2021

@alexezeder, Can you make a pull request?

@alexezeder
Copy link
Contributor

alexezeder commented Apr 27, 2021 •
edited
Loading

Actually I already somehow fixed this with 77258f6:

- #if __cplusplus >= 201703L
+ #if __cplusplus >= 201703L || (defined(_MSVC_LANG) && _MSVC_LANG >= 201703L)

Looking at your proposal, probably it's not enough since you have an additional _MSC_VER >= 1914 check there.

Can you make a pull request?

Of course I can, but it's not needed right now because, as I said, I make it work for my PR. Feel free to make PR at your convenience.

@phprus
Copy link
Contributor Author

phprus commented Apr 27, 2021

_MSVC_LANG >= 201703L - VS2017 version 15.3+
constexpr char_traits - VS 2017 version 15.7+

@phprus phprus mentioned this pull request Apr 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexezeder alexezeder alexezeder requested changes

@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
@phprus @vitaut @alexezeder