-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Added support for pre-c++17 experimental string_view #607
Added support for pre-c++17 experimental string_view #607
Conversation
FIxed one wrong #if FMT_HAS_(EXPERIMENTAL_)STRING_VIEW added in commit before |
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 minor comments inline.
fmt/format.h
Outdated
#else | ||
# define FMT_HAS_STRING_VIEW 0 |
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 keeping this and removing 2 identical defines below since they don't depend on whether experimental string_view exists.
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.
Can't get your point, can you paste here how you want the final code to look like?
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.
#else
# define FMT_HAS_STRING_VIEW 0
# if (FMT_HAS_INCLUDE(<experimental/string_view>) && __cplusplus > 201402L)
# include <experimental/string_view>
# define FMT_HAS_EXPERIMENTAL_STRING_VIEW 1
# else
# define FMT_HAS_EXPERIMENTAL_STRING_VIEW 0
# endif
@@ -582,6 +590,26 @@ class BasicStringRef { | |||
} | |||
#endif | |||
|
|||
#if FMT_HAS_EXPERIMENTAL_STRING_VIEW | |||
/** | |||
\rst |
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.
nit: Please indent the rst block for consistency with the rest of the code.
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.
ok
test/CMakeLists.txt
Outdated
@@ -11,6 +11,10 @@ target_compile_options(gmock PUBLIC ${CPP11_FLAG}) | |||
target_compile_definitions(gmock PUBLIC GTEST_HAS_STD_WSTRING=1) | |||
target_include_directories(gmock PUBLIC .) | |||
|
|||
if (CYGWIN) | |||
target_compile_definitions(gmock PUBLIC _POSIX_C_SOURCE=200809) |
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 is this for?
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.
This is because google test fails to run on Cygwin because they doesn't define _POSIX_C_SOURCE and posix api´s (fileno, strdup, fdopen, and others).
See reference https://stackoverflow.com/questions/18784112/googletest-1-6-with-cygwin-1-7-compile-error-fileno-was-not-declared-in-this
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.
Could you add a comment explaining this?
Comments about CYGWIN tests for building
Take a look at this last commit, if it fits your requests. |
Merged, thanks! |
Also, added _POSIX_C_SOURCE to make library compile and pass on all tests on Cygwin environment.