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.

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

Make ranges-test available with C++11 #2114

Merged

Conversation

alexezeder
Copy link
Contributor

I noticed here this strange check:

fmt/test/ranges-test.cc

Lines 16 to 18 in acef0bb

// Check if 'if constexpr' is supported.
#if (__cplusplus > 201402L) || \
(defined(_MSVC_LANG) && _MSVC_LANG > 201402L && _MSC_VER >= 1910)

So I decided to do it right - by using __cpp_if_constexpr feature macro. But later I realized that since there is no mention in docs that ranges are supported only with C++14 (or C++17 because of constexpr if) it's strange to me that test doesn't check C++11, especially when this check is not necessary at all with only few changes in the test.
With this PR ranges-test compiles with C++11 on probably most ancient supported compilers - gcc-4.8 and clang-3.5 (couldn't test 3.4).

Copy link
Contributor Author

@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.

Some points for the PR.

@@ -381,7 +381,7 @@ template <typename T> inline T* make_checked(T* p, size_t) { return p; }
#endif

template <typename Container, FMT_ENABLE_IF(is_contiguous<Container>::value)>
#if FMT_CLANG_VERSION
#if FMT_CLANG_VERSION >= 307
__attribute__((no_sanitize("undefined")))
Copy link
Contributor Author

@alexezeder alexezeder Jan 23, 2021

Choose a reason for hiding this comment

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

Looks like there is no no_sanitize attribute prior to Clang 3.7
Maybe a bit unrelated to this PR, but it was found while I was working on it

static constexpr const char left_padding_shifts[] = {31, 31, 0, 1, 0};
static constexpr const char right_padding_shifts[] = {0, 31, 0, 1, 0};
static constexpr const char left_padding_shifts[5] = {31, 31, 0, 1, 0};
static constexpr const char right_padding_shifts[5] = {0, 31, 0, 1, 0};
Copy link
Contributor Author

@alexezeder alexezeder Jan 23, 2021

Choose a reason for hiding this comment

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

Some GCC versions fail to determine the size of this array - https://godbolt.org/z/KbhMGb
Maybe a bit unrelated to this PR, but it was found while I was working on it

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -368,7 +368,7 @@ template <typename Char, typename... T> struct tuple_arg_join : detail::view {
basic_string_view<Char> sep;

tuple_arg_join(const std::tuple<T...>& t, basic_string_view<Char> s)
: tuple{t}, sep{s} {}
: tuple(t), sep{s} {}
Copy link
Contributor Author

@alexezeder alexezeder Jan 23, 2021

Choose a reason for hiding this comment

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

GCC 4.8 fails to initialize tuple ref by braces - https://godbolt.org/z/qxq5eK

# include <vector>
#if FMT_GCC_VERSION == 0 || FMT_GCC_VERSION >= 601
# define FMT_ENABLE_C_STYLE_ARRAY_TEST
#endif
Copy link
Contributor Author

@alexezeder alexezeder Jan 23, 2021

Choose a reason for hiding this comment

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

It's strange but looks like GCC <6 unable to pass C-style array correctly, so {fmt} is unable right now to format them - https://godbolt.org/z/MYYf6W

}
template <size_t N>
fmt::enable_if_t<N == 1, fmt::string_view> get() const noexcept {
return {str};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No C++14 decltype(auto) (probably was unneeded anyway) and C++17 constexpr if, only C++11 code

@@ -125,7 +131,7 @@ TEST(RangesTest, FormatStruct) {

TEST(RangesTest, FormatTo) {
char buf[10];
auto end = fmt::format_to(buf, "{}", std::vector{1, 2, 3});
auto end = fmt::format_to(buf, "{}", std::vector<int>{1, 2, 3});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No C++17 template deduction

@vitaut vitaut merged commit 2a25e2b into fmtlib:master Jan 30, 2021
@vitaut
Copy link
Contributor

vitaut commented Jan 30, 2021

Looks great, thanks!

@alexezeder alexezeder deleted the fix/if_constexpr_guard_in_ranges_test branch February 7, 2021 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants