8000
Skip to content

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

Merged
vitaut merged 2 commits intofmtlib:masterfrom
alexezeder:fix/if_constexpr_guard_in_ranges_test
Jan 30, 2021
Merged

Make ranges-test available with C++11#2114
vitaut merged 2 commits intofmtlib:masterfrom
alexezeder:fix/if_constexpr_guard_in_ranges_test

Conversation

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

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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


tuple_arg_join(const std::tuple<T...>& t, basic_string_view<Char> s)
: tuple{t}, sep{s} {}
: tuple(t), sep{s} {}
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

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
Copy Markdown
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
Copy Markdown
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

0