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

Add tests for FMT_ENFORCE_COMPILE_STRING, fix several errors #2038

Merged
merged 34 commits into from
Dec 24, 2020
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
e05c8fc
Add test
Nov 30, 2020
f76c64a
fix wide string formatting
Nov 30, 2020
b75bd49
fix formatted_size
Nov 30, 2020
2f9484c
fix chrono
Nov 30, 2020
3b6d21b
clang-format, test fixes
Nov 30, 2020
aa65e99
fix ranges
Nov 30, 2020
54d6d5b
fix gcc test
Nov 30, 2020
e805e8e
workaround MSVC bug
Nov 30, 2020
1fbfccc
Improve docs
Nov 30, 2020
55600c9
fix ranges.h
Nov 30, 2020
35b94fa
increase MSVC version restriction
Nov 30, 2020
1e034bd
try a fix for strange gcc failures
Dec 1, 2020
900adeb
fix gcc pedantic error
Dec 1, 2020
7f1c041
fix gcc errors
Dec 1, 2020
d343e4b
fix MSVC version check #if statements
Dec 1, 2020
c4c8351
trim down compiletime test
Dec 4, 2020
60bceae
fixup chrono.h with better notes about MSVC workaround
Dec 4, 2020
b61537a
restore missing include
Dec 4, 2020
d539f76
cleanup doc string
Dec 8, 2020
dafbc05
fixup test
Dec 8, 2020
9fafb14
fix unused arg issue in test
Dec 8, 2020
2489501
fix gcc issue
Dec 8, 2020
b8ebadd
fixup chrono MSVC workaround
Dec 9, 2020
098de0d
Remove formatted_size overlaod accepting compile-time strings
Dec 12, 2020
8e575bd
address comments
Dec 13, 2020
f2a5a0f
restore ifdef guarding GCC bug
Dec 13, 2020
c85d5e3
address easy comments
Dec 21, 2020
b9063f9
remove vformat from chrono.h
Dec 21, 2020
98a8830
attempt stripping void casts
Dec 21, 2020
b0c65df
Refactor ranges, fix chrono, apply clang format to include/fmt/*.h
Dec 21, 2020
6f393f2
fixup constexpr issues
Dec 21, 2020
06a576c
revert locale format fix
Dec 24, 2020
99ed2a8
address feedback
Dec 24, 2020
5083486
move array of literals format test
Dec 24, 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
5 changes: 5 additions & 0 deletions doc/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,11 @@ functions in their ``formatter`` specializations.

.. _udt:

To force the use of compile-time checks, define the preprocessor variable
``FMT_ENFORCE_COMPILE_STRING``. When set, functions accepting ``FMT_STRING``
will fail to compile with regular strings. Runtime-checked
formatting is still possible using ``fmt::vformat``, ``fmt::vprint``, etc.

Formatting User-defined Types
-----------------------------

Expand Down
34 changes: 19 additions & 15 deletions include/fmt/chrono.h
Original file line number Diff line number Diff line change
Expand Up @@ -768,19 +768,16 @@ inline std::chrono::duration<Rep, std::milli> get_milliseconds(
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);
return write<Char>(out, 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) {
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);
basic_format_specs<Char> specs;
specs.precision = precision;
specs.type = precision > 0 ? 'f' : 'g';
return write<Char>(out, val, specs);
}

template <typename Char, typename OutputIt>
Expand All @@ -800,13 +797,20 @@ 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());
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);

basic_memory_buffer<Char> buffer;
auto bufOut = std::back_inserter(buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary. You can write to out directly.

*bufOut++ = '[';
bufOut = write<Char>(bufOut, Period::num);

if (const_check(Period::den != 1)) {
*bufOut++ = '/';
bufOut = write<Char>(bufOut, Period::den);
}

*bufOut++ = ']';
*bufOut++ = 's';
return write<Char>(out, {buffer.data(), buffer.size()});
}

template <typename FormatContext, typename OutputIt, typename Rep,
Expand Down
2 changes: 1 addition & 1 deletion include/fmt/format.h
Original file line number Diff line number Diff line change
Expand Up @@ -3828,7 +3828,7 @@ inline std::string to_string(T value) {
Converts *value* to ``std::wstring`` using the default format for type *T*.
*/
template <typename T> inline std::wstring to_wstring(const T& value) {
return format(L"{}", value);
return format(FMT_STRING(L"{}"), value);
}

template <typename Char, size_t SIZE>
Expand Down
18 changes: 9 additions & 9 deletions include/fmt/ranges.h
Original file line number Diff line number Diff line change
Expand Up @@ -291,10 +291,10 @@ struct formatter<TupleT, Char, enable_if_t<fmt::is_tuple_like<TupleT>::value>> {
}
out = detail::copy(formatting.delimiter, out);
}
out = format_to(out,
detail::format_str_quoted(
(formatting.add_delimiter_spaces && i > 0), v),
v);
out = vformat_to(out,
detail::format_str_quoted(
(formatting.add_delimiter_spaces && i > 0), v),
make_format_args<FormatContext>(v));
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below: it would be better to refactor this to use compile string and format_to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered that but given that the format string used appeared to be determined at run-time I wasn't sure it was possible. I'll give it a go.

Copy link
Contributor Author

@yeswalrus yeswalrus Dec 21, 2020

Choose a reason for hiding this comment

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

Looking at this more I'm reaching the same conclusion - formatting can't be used from within FMT_STRING - doing so causes

/home/walter.gray/development/walrus/fmt/include/fmt/ranges.h:371:29: error: use of non-static data member 'formatting' of 'formatter<T, Char, enable_if_t<fmt::is_range<T, Char>::value && (has_formatter<detail::value_type<T>, format_context>::value || detail::has_fallback_formatter<detail::value_type<T>, format_context>::value)> >' from nested type 'FMT_COMPILE_STRING'
                           (formatting.add_delimiter_spaces && i > 0), *it)),

I couldn't have a function that returned one of several format strings, and I don't think I could have a function that returned a constexpr string that would work with FMT_STRING either -

constexpr const char* getString() {
    return "{} bananna";
}
int main(int argc, char** argv) {
    fmt::print(FMT_STRING(getString()), "fried");
}

for example will not compile in godbolt. I also cant return the result of FMT_STRING since they'd be different types between branches. My best guess for how to do this would be to have a wrapping function that used template specialization on a bool for add_delimiter_spaces and runtime switching on i > 0 and called format_to with the appropriate FMT_STRING directly. Am I missing an option here or should I try that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use write instead of format_to here too.

++i;
}

Expand Down Expand Up @@ -366,12 +366,12 @@ struct formatter<
if (formatting.add_prepostfix_space) *out++ = ' ';
out = detail::copy(formatting.delimiter, out);
}
out = format_to(out,
detail::format_str_quoted(
(formatting.add_delimiter_spaces && i > 0), *it),
*it);
out = vformat_to(out,
detail::format_str_quoted(
(formatting.add_delimiter_spaces && i > 0), *it),
make_format_args<FormatContext>(*it));
if (++i > formatting.range_length_limit) {
out = format_to(out, " ... <other elements>");
out = format_to(out, FMT_STRING("{}"), " ... <other elements>");
break;
}
}
Expand Down
8 changes: 8 additions & 0 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,14 @@ add_fmt_test(printf-test)
add_fmt_test(ranges-test)
add_fmt_test(scan-test)

if (NOT MSVC)
# FMT_ENFORCE_COMPILE_STRING not supported under MSVC
# See https://developercommunity.visualstudio.com/content/problem/1277597/internal-compiler-c0001-error-on-complex-nested-la.html
add_fmt_test(enforce-compile-string-test)
target_compile_definitions(enforce-compile-string-test PRIVATE
"-DFMT_ENFORCE_COMPILE_STRING")
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto


if (NOT DEFINED MSVC_STATIC_RUNTIME AND MSVC)
foreach (flag_var
CMAKE_CXX_FLAGS CMAKE_CXX_FLAGS_DEBUG CMAKE_CXX_FLAGS_RELEASE
Expand Down
75 changes: 75 additions & 0 deletions test/enforce-compile-string-test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// Formatting library for C++ - formatting library tests
//
// Copyright (c) 2012 - present, Victor Zverovich
// All rights reserved.
//
// For the license information refer to format.h.

#include <array>
#include <chrono>
#include <iterator>
#include <list>
#include <string>

#include "fmt/chrono.h"
#include "fmt/color.h"
#include "fmt/format.h"
#include "fmt/locale.h"
#include "fmt/ostream.h"
#include "fmt/ranges.h"

// Exercise the API to verify that everything we expect to can compile.
void test_format_api() {
fmt::format(FMT_STRING("{}"), 42);
fmt::format(FMT_STRING(L"{}"), 42);
fmt::format(FMT_STRING("noop"));

fmt::to_string(42);
fmt::to_wstring(42);

std::list<char> out;
fmt::format_to(std::back_inserter(out), FMT_STRING("{}"), 42);

char buffer[4];
fmt::format_to_n(buffer, 3, FMT_STRING("{}"), 12345);

wchar_t wbuffer[4];
fmt::format_to_n(wbuffer, 3, FMT_STRING(L"{}"), 12345);
}

void test_literals_api() {
#if FMT_USE_UDL_TEMPLATE
using namespace fmt::literals;
"{}c{}"_format("ab", 1);
L"{}c{}"_format(L"ab", 1);
#endif
}

void test_chrono() {
fmt::format(FMT_STRING("{}"), std::chrono::seconds(42));
fmt::format(FMT_STRING(L"{}"), std::chrono::seconds(42));
}

void test_text_style() {
fmt::print(fg(fmt::rgb(255, 20, 30)), FMT_STRING("{}"), "rgb(255,20,30)");
fmt::format(fg(fmt::rgb(255, 20, 30)), FMT_STRING("{}"),
"rgb(255,20,30)");

fmt::text_style ts = fg(fmt::rgb(255, 20, 30));
std::string out;
fmt::format_to(std::back_inserter(out), ts,
FMT_STRING("rgb(255,20,30){}{}{}"), 1, 2, 3);
}

void test_range() {
std::array<char, 5> hello = {'h','e','l','l','o'};
Copy link
Contributor

Choose a reason for hiding this comment

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

Please apply clang-format.

fmt::format(FMT_STRING("{}"), hello);
}

int main() {
test_format_api();
test_literals_api();
test_chrono();
test_text_style();
test_range();
}