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

Fixed GCC version test. #771

Closed
wants to merge 6 commits into from
Closed

Fixed GCC version test. #771

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
120a969
Added quotes for strings in ranges and tuple likes.
Remotion Jun 7, 2018
85fc14a
Fixed line length.
Remotion Jun 8, 2018
8d0e261
Fixed GCC version test.
Remotion Jun 8, 2018
5b2b804
Added support for rgb color output.
Remotion Jun 7, 2018
210e0a4
Using enum class now.
Remotion Jun 8, 2018
d102cfd
Document that file should be in wide-oriented mode for wide print
vitaut Jun 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/fmt/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
# define FMT_ICC_VERSION 0
#endif

#if (defined(__GNUC__) && !defined(__clang__) && __GNUC__ >= 406) || \
#if (defined(__GNUC__) && !defined(__clang__) && (__GNUC__ * 100 + __GNUC_MINOR__) >= 406) || \
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could use FMT_GCC_VERSION instead of __GNUC__ * 100 + __GNUC_MINOR__ (possibly after reordering defines).

Copy link
Contributor

@eliaskosunen eliaskosunen Jun 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

FMT_GCC_VERSION is defined in core.h, which is included after this. This also needs to be before the inclusion of core.h to disable warnings from that header. On your first review in #736 you said that FMT_GCC_VERSION needs to be in core.h.

So that leaves two options that I can think of:

  1. Leave it as is
  2. Duplicate these warning-disabling maneuvers in both core.h and format.h and move the inclusion of core.h before the disabling of the warnings in format.h, so both can use FMT_GCC_VERSION, like this:
// core.h
#define FMT_GCC_VERSION /* ... */

#if FMT_GCC_VERSION
// disable
#endif

// ...

#if FMT_GCC_VERSION
// re-enable
#endif
// format.h

#include "core.h"

#if FMT_GCC_VERSION
// disable
#endif

// ...

#if FMT_GCC_VERSION
// re-enable
#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 said that very incoherently. What I was trying to say was that the warnings need to be disabled before the inclusion of core.h, but FMT_GCC_VERSION is defined in core.h. So we can either not use FMT_GCC_VERSION in disabling the warnings or disable them in both headers. The latter would probably be preferable, because now the warnings aren't disabled at all if you only include core.h.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can live without disabling these warnings in fmt/core.h for now, so I suggest just moving fmt/core.h to the front and using FMT_GCC_VERSION here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The original point of this PR was to fix the version check so there wouldn't be any -Wshadow warnings.

Copy link
Contributor

@vitaut vitaut Jun 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes and this will fix the reported warnings in fmt/format.h (but not in fmt/core.h which is OK since no such warnings were reported).

FMT_CLANG_VERSION
# pragma GCC diagnostic push

Expand Down
47 changes: 37 additions & 10 deletions include/fmt/ranges.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,35 @@ void for_each(Tuple &&tup, F &&f) {
const auto indexes = get_indexes(tup);
for_each(indexes, std::forward<Tuple>(tup), std::forward<F>(f));
}

template<typename Arg>
FMT_CONSTEXPR const char* format_str_quoted(bool add_space, const Arg&,
typename std::enable_if<
!is_like_std_string<typename std::decay<Arg>::type>::value>::type* = nullptr) {
return add_space ? " {}" : "{}";
}

template<typename Arg>
FMT_CONSTEXPR const char* format_str_quoted(bool add_space, const Arg&,
typename std::enable_if<
is_like_std_string<typename std::decay<Arg>::type>::value>::type* = nullptr) {
return add_space ? " \"{}\"" : "\"{}\"";
}

FMT_CONSTEXPR const char* format_str_quoted(bool add_space, const char*) {
return add_space ? " \"{}\"" : "\"{}\"";
}
FMT_CONSTEXPR const wchar_t* format_str_quoted(bool add_space, const wchar_t*) {
return add_space ? L" \"{}\"" : L"\"{}\"";
}

FMT_CONSTEXPR const char* format_str_quoted(bool add_space, const char) {
return add_space ? " '{}'" : "'{}'";
}
FMT_CONSTEXPR const wchar_t* format_str_quoted(bool add_space, const wchar_t) {
return add_space ? L" '{}'" : L"'{}'";
}

} // namespace internal

template <typename T>
Expand All @@ -185,11 +214,10 @@ struct formatter<TupleT, Char,
}
internal::copy(formatting.delimiter, out);
}
if (formatting.add_delimiter_spaces && i > 0) {
format_to(out, " {}", v);
} else {
format_to(out, "{}", v);
}
format_to(out,
internal::format_str_quoted(
(formatting.add_delimiter_spaces && i > 0), v),
v);
++i;
}

Expand Down Expand Up @@ -252,11 +280,10 @@ struct formatter<RangeT, Char,
}
internal::copy(formatting.delimiter, out);
}
if (formatting.add_delimiter_spaces && i > 0) {
format_to(out, " {}", *it);
} else {
format_to(out, "{}", *it);
}
format_to(out,
internal::format_str_quoted(
(formatting.add_delimiter_spaces && i > 0), *it),
*it);
if (++i > formatting.range_length_limit) {
format_to(out, " ... <other elements>");
break;
Expand Down
10 changes: 5 additions & 5 deletions test/ranges-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ TEST(RangesTest, FormatVector2) {

TEST(RangesTest, FormatMap) {
std::map<std::string, int32_t> simap{{"one", 1}, {"two", 2}};
EXPECT_EQ("{(one, 1), (two, 2)}", fmt::format("{}", simap));
EXPECT_EQ("{(\"one\", 1), (\"two\", 2)}", fmt::format("{}", simap));
}

TEST(RangesTest, FormatPair) {
Expand All @@ -41,9 +41,9 @@ TEST(RangesTest, FormatPair) {
}

TEST(RangesTest, FormatTuple) {
std::tuple<int64_t, float, std::string> tu1{42, 3.14159265358979f,
"this is tuple"};
EXPECT_EQ("(42, 3.14159, this is tuple)", fmt::format("{}", tu1));
std::tuple<int64_t, float, std::string, char> tu1{42, 3.14159265358979f,
"this is tuple", 'i'};
EXPECT_EQ("(42, 3.14159, \"this is tuple\", 'i')", fmt::format("{}", tu1));
}

/// Check if 'if constexpr' is supported.
Expand Down Expand Up @@ -81,7 +81,7 @@ struct tuple_element<N, my_struct> {

TEST(RangesTest, FormatStruct) {
my_struct mst{13, "my struct"};
EXPECT_EQ("(13, my struct)", fmt::format("{}", mst));
EXPECT_EQ("(13, \"my struct\")", fmt::format("{}", mst));
}

#endif // (__cplusplus > 201402L) || (defined(_MSVC_LANG) && _MSVC_LANG >
Expand Down