8000
Skip to content

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

Merged
vitaut merged 1 commit intofmtlib:masterfrom
phprus:constexpr-char-traits
Apr 23, 2021
Merged

C++17: std::char_traits<>::{compare,length} is constexpr.#2246
vitaut merged 1 commit intofmtlib:masterfrom
phprus:constexpr-char-traits

Conversation

@phprus
Copy link
Copy Markdown
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
Copy Markdown
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
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
Copy Markdown
Contributor
@alexezeder alexezeder Apr 21, 2021

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
Copy Markdown
Contributor
@alexezeder alexezeder Apr 21, 2021

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
Copy Markdown
Contributor Author
@phprus phprus Apr 21, 2021

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
Copy Markdown
Contributor
@alexezeder alexezeder Apr 21, 2021

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
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
Copy Markdown
Contributor
@alexezeder alexezeder Apr 21, 2021

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
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
Copy Markdown
Contributor
@alexezeder alexezeder Apr 21, 2021

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contrib 8000 utor
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
Copy Markdown
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
Copy Markdown
Contributor Author
phprus commented Apr 27, 2021

@alexezeder, Can you make a pull request?

@alexezeder
Copy link
Copy Markdown
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
Copy Markdown
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

@vitaut vitaut vitaut approved these changes

+1 more reviewer

@alexezeder alexezeder alexezeder requested changes

Reviewers whose approvals may not affect merge requirements

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
0