-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Changes to make {fmt} to play nicer in existing projects. #656
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Mostly looks good, just a few small comments inline.
include/fmt/core.h
Outdated
@@ -580,7 +580,7 @@ FMT_MAKE_VALUE(FMT_POINTER, std::nullptr_t, const void*) | |||
// formatting of "[const] volatile char *" which is printed as bool by | |||
// iostreams. | |||
template <typename T> | |||
void make_value(const T *p) { | |||
void make_value(const T */*p*/) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest just removing p
. Also "decuction" -> "deduction" in the commit title.
include/fmt/core.h
Outdated
DOUBLE, LONG_DOUBLE, LAST_NUMERIC_TYPE = LONG_DOUBLE, | ||
CSTRING, STRING, POINTER, CUSTOM | ||
FMT_DOUBLE, FMT_LONG_DOUBLE, FMT_LAST_NUMERIC_TYPE = FMT_LONG_DOUBLE, | ||
FMT_CSTRING, FMT_STRING, FMT_POINTER, FMT_CUSTOM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding FMT_
prefix is no longer needed as the constants have been renamed in 8ed264f.
@@ -1910,7 +1910,7 @@ FMT_CONSTEXPR bool test_error(const char *fmt, const char *expected_error) { | |||
TEST(FormatTest, FormatStringErrors) { | |||
EXPECT_ERROR("foo", nullptr); | |||
EXPECT_ERROR("}", "unmatched '}' in format string"); | |||
EXPECT_ERROR("{0:s", "unknown format specifier", Date); | |||
//EXPECT_ERROR("{0:s", "unknown format specifier", Date); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the full error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the error message to the commit message. Also here:
/home/lgb/src/cppformat/test/format-test.cc: In member function ‘virtual void FormatTest_FormatStringErrors_Test::TestBody()’:
/home/lgb/src/cppformat/test/format-test.cc:1919:3: error: non-constant condition for static assertion
static_assert(test_error<VA_ARGS>(fmt, error), "")
^
/home/lgb/src/cppformat/test/format-test.cc:1924:3: note: in expansion of macro ‘EXPECT_ERROR’
EXPECT_ERROR("{0:s", "unknown format specifier", Date);
^~~~~~~~~~~~
include/fmt/format.h
Outdated
@@ -906,7 +906,7 @@ inline Char *format_decimal(Char *buffer, UInt value, unsigned num_digits, | |||
|
|||
template <typename UInt, typename Iterator, typename ThousandsSep> | |||
inline Iterator format_decimal( | |||
Iterator out, UInt value, unsigned num_digits, ThousandsSep thousands_sep) { | |||
Iterator out, UInt value, unsigned num_digits, ThousandsSep /*thousands_sep*/) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest just removing thousands_sep
instead of commenting out.
7c4619c
to
26d3a22
Compare
Found with GCC and -Wzero-as-null-pointer-constant
C++11 does not support deduction of return type.
GCC 7.3 complains about uninitialized varaibles in constexpr context.
GCC 7.3 complains that this is not a compile time constant. test/format-test.cc: In member function ‘virtual void FormatTest_FormatStringErrors_Test::TestBody()’: test/format-test.cc:1919:3: error: non-constant condition for static assertion static_assert(test_error<__VA_ARGS__>(fmt, error), "") ^ test/format-test.cc:1924:3: note: in expansion of macro ‘EXPECT_ERROR’ EXPECT_ERROR("{0:s", "unknown format specifier", Date); ^~~~~~~~~~~~
Merged, thanks! |
Use FMT_NULL a couple of more places, we compile our code with -wzero-as-null-pointer-constant
Some of the enumerators clash with old(ish) C macros/defines.
Last test patch is somehting that you probably do not want.