Skip to content

Always use FMT_STRING internally where possible [Issue #2002] #2006

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

Merged
vitaut merged 13 commits into from
Nov 15, 2020
Merged

Always use FMT_STRING internally where possible [Issue #2002] #2006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9501da5
Always use FMT_STRING internally where possible
Nov 10, 2020
854bf9a
fixed chrono.h changes
Nov 11, 2020
67a4447
fixup type errors
Nov 11, 2020
969dcce
revert removal of const
Nov 12, 2020
45c7c29
use constexpr
Nov 13, 2020
eefa1e5
use FMT_CONSTEXPR_DECL
Nov 13, 2020
93f1e22
use FMT_ENABLE_IF for compile-time if branching
Nov 13, 2020
03c29de
ignore unusable param in non-float overload
Nov 14, 2020
2c271ac
workaround MSVC bug, add test
Nov 14, 2020
45b9f96
#ifdef test broken in MSVC
Nov 14, 2020
98a3557
improve test, fix conditional
Nov 14, 2020
fc3832f
attempt simplification to work around MSVC problem
Nov 14, 2020
d5e3e34
clang-format
Nov 14, 2020
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
34 changes: 23 additions & 11 deletions include/fmt/chrono.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,15 +762,24 @@ inline std::chrono::duration<Rep, std::milli> get_milliseconds(
return std::chrono::duration<Rep, std::milli>(static_cast<Rep>(ms));
}

template <typename Char, typename Rep, typename OutputIt>
template <typename Char, typename Rep, typename OutputIt,
FMT_ENABLE_IF(std::is_integral<Rep>::value)>
OutputIt format_duration_value(OutputIt out, Rep val, int) {
static FMT_CONSTEXPR_DECL const Char format[] = {'{', '}', 0};
return format_to(out, compile_string_to_view(format), val);
}

template <typename Char, typename Rep, typename OutputIt,
FMT_ENABLE_IF(std::is_floating_point<Rep>::value)>
OutputIt format_duration_value(OutputIt out, Rep val, int precision) {
const Char pr_f[] = {'{', ':', '.', '{', '}', 'f', '}', 0};
if (precision >= 0) return format_to(out, pr_f, val, precision);
const Char fp_f[] = {'{', ':', 'g', '}', 0};
const Char format[] = {'{', '}', 0};
return format_to(out, std::is_floating_point<Rep>::value ? fp_f : format,
val);
static FMT_CONSTEXPR_DECL const Char pr_f[] = {'{', ':', '.', '{',
'}', 'f', '}', 0};
if (precision >= 0)
return format_to(out, compile_string_to_view(pr_f), val, precision);
static FMT_CONSTEXPR_DECL const Char fp_f[] = {'{', ':', 'g', '}', 0};
return format_to(out, compile_string_to_view(fp_f), val);
}

template <typename Char, typename OutputIt>
OutputIt copy_unit(string_view unit, OutputIt out, Char) {
return std::copy(unit.begin(), unit.end(), out);
Expand All @@ -788,10 +797,13 @@ template <typename Char, typename Period, typename OutputIt>
OutputIt format_duration_unit(OutputIt out) {
if (const char* unit = get_units<Period>())
return copy_unit(string_view(unit), out, Char());
const Char num_f[] = {'[', '{', '}', ']', 's', 0};
if (const_check(Period::den == 1)) return format_to(out, num_f, Period::num);
const Char num_def_f[] = {'[', '{', '}', '/', '{', '}', ']', 's', 0};
return format_to(out, num_def_f, Period::num, Period::den);
static FMT_CONSTEXPR_DECL const Char num_f[] = {'[', '{', '}', ']', 's', 0};
if (const_check(Period::den == 1))
return format_to(out, compile_string_to_view(num_f), Period::num);
static FMT_CONSTEXPR_DECL const Char num_def_f[] = {'[', '{', '}', '/', '{',
'}', ']', 's', 0};
return format_to(out, compile_string_to_view(num_def_f), Period::num,
Period::den);
}

template <typename FormatContext, typename OutputIt, typename Rep,
Expand Down
15 changes: 8 additions & 7 deletions include/fmt/format-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ FMT_FUNC void format_error_code(detail::buffer<char>& out, int error_code,
error_code_size += detail::to_unsigned(detail::count_digits(abs_value));
auto it = buffer_appender<char>(out);
if (message.size() <= inline_buffer_size - error_code_size)
format_to(it, "{}{}", message, SEP);
format_to(it, "{}{}", ERROR_STR, error_code);
format_to(it, FMT_STRING("{}{}"), message, SEP);
format_to(it, FMT_STRING("{}{}"), ERROR_STR, error_code);
assert(out.size() <= inline_buffer_size);
}

Expand Down Expand Up @@ -2662,14 +2662,15 @@ template <> struct formatter<detail::bigint> {
for (auto i = n.bigits_.size(); i > 0; --i) {
auto value = n.bigits_[i - 1u];
if (first) {
out = format_to(out, "{:x}", value);
out = format_to(out, FMT_STRING("{:x}"), value);
first = false;
continue;
}
out = format_to(out, "{:08x}", value);
out = format_to(out, FMT_STRING("{:08x}"), value);
}
if (n.exp_ > 0)
out = format_to(out, "p{}", n.exp_ * detail::bigint::bigit_bits);
out = format_to(out, FMT_STRING("p{}"),
n.exp_ * detail::bigint::bigit_bits);
return out;
}
};
Expand Down Expand Up @@ -2715,8 +2716,8 @@ FMT_FUNC void format_system_error(detail::buffer<char>& out, int error_code,
int result =
detail::safe_strerror(error_code, system_message, buf.size());
if (result == 0) {
format_to(detail::buffer_appender<char>(out), "{}: {}", message,
system_message);
format_to(detail::buffer_appender<char>(out), FMT_STRING("{}: {}"),
message, system_message);
return;
}
if (result != ERANGE)
Expand Down
33 changes: 24 additions & 9 deletions test/format-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,22 +355,20 @@ TEST(MemoryBufferTest, ExceptionInDeallocate) {
}

template <typename Allocator, size_t MaxSize>
class allocator_max_size: public Allocator {
class allocator_max_size : public Allocator {
public:
using typename Allocator::value_type;
size_t max_size() const FMT_NOEXCEPT {
return MaxSize;
}
size_t max_size() const FMT_NOEXCEPT { return MaxSize; }
value_type* allocate(size_t n) {
if (n > max_size()) {
throw std::length_error("size > max_size");
}
return std::allocator_traits<Allocator>::allocate(
*static_cast<Allocator *>(this), n);
*static_cast<Allocator*>(this), n);
}
void deallocate(value_type* p, size_t n) {
std::allocator_traits<Allocator>::deallocate(
*static_cast<Allocator *>(this), p, n);
std::allocator_traits<Allocator>::deallocate(*static_cast<Allocator*>(this),
p, n);
}
};

Expand All @@ -383,7 +381,7 @@ TEST(MemoryBufferTest, AllocatorMaxSize) {
try {
// new_capacity = 128 + 128/2 = 192 > 160
buffer.resize(160);
} catch (const std::exception &) {
} catch (const std::exception&) {
throws_on_resize = true;
}
EXPECT_FALSE(throws_on_resize);
Expand All @@ -395,7 +393,7 @@ TEST(MemoryBufferTest, AllocatorMaxSizeOverflow) {
bool throws_on_resize = false;
try {
buffer.resize(161);
} catch (const std::exception &) {
} catch (const std::exception&) {
throws_on_resize = true;
}
EXPECT_TRUE(throws_on_resize);
Expand Down Expand Up @@ -1807,11 +1805,28 @@ fmt::string_view to_string_view(string_like) { return "foo"; }

constexpr char with_null[3] = {'{', '}', '\0'};
constexpr char no_null[2] = {'{', '}'};
static FMT_CONSTEXPR_DECL const char static_with_null[3] = {'{', '}', '\0'};
static FMT_CONSTEXPR_DECL const wchar_t static_with_null_wide[3] = {'{', '}',
'\0'};
static FMT_CONSTEXPR_DECL const char static_no_null[2] = {'{', '}'};
static FMT_CONSTEXPR_DECL const wchar_t static_no_null_wide[2] = {'{', '}'};

TEST(FormatTest, CompileTimeString) {
EXPECT_EQ("42", fmt::format(FMT_STRING("{}"), 42));
EXPECT_EQ(L"42", fmt::format(FMT_STRING(L"{}"), 42));
EXPECT_EQ("foo", fmt::format(FMT_STRING("{}"), string_like()));

(void)static_with_null;
(void)static_with_null_wide;
(void)static_no_null;
(void)static_no_null_wide;
#if !defined(_MSC_VER)
EXPECT_EQ("42", fmt::format(FMT_STRING(static_with_null), 42));
EXPECT_EQ(L"42", fmt::format(FMT_STRING(static_with_null_wide), 42));
EXPECT_EQ("42", fmt::format(FMT_STRING(static_no_null), 42));
EXPECT_EQ(L"42", fmt::format(FMT_STRING(static_no_null_wide), 42));
#endif
Comment on lines +1819 to +1828

@vitaut vitaut Nov 14, 2020

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.

I don't think we need this because we already test this below.

@yeswalrus yeswalrus Nov 14, 2020 •
edited
Loading

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.

Not quite - you test with constexpr locals, which are a c++ 17(14?) feature, but static constexpr char arrays are valid all the way back to c++11 and this exposes a bug in MSVC.


(void)with_null;
(void)no_null;
#if __cplusplus >= 201703L
Expand Down