-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Basic string_view support #1217
base: master
Are you sure you want to change the base?
Conversation
independent of UNICODE vs MBCS setting Set CPPREST_FORCE_NARROW_STRINGS=ON to enable. Simplistic cleanup throughout project is not possible as some Windows APIs require utf16 such as HTTPSPolicyCallbackData in http_client_winhttp.cpp. HTTP related code is not completely updated so a full package may not be produced, though settings exist to have a clean build. Use: -DCPPREST_FORCE_NARROW_STRING=ON -DCPPREST_EXCLUDE_WEBSOCKETS=ON -DCPPREST_HTTP_LISTENER_IMPL=none -DBUILD_SAMPLES=OFF
…omponents like just json support extractions from asyncrt_utils.h: base64_utils.h (to/form base64) memory_utils.h (details::make_unique) string_utils.cpp (string utils) asyncrt_utils retains error, date-time, and nounce utils
- default to enable CPPREST_USE_STRING_VIEWS when compiler supports C++ 17 most string utility and json APIs are changed to support string_views (deprecated APIs left alone) primary argument replacements: const utility::string_t& -> utility::string_view_t const std::string& -> utility::nstring_view_t const std::wstring& -> utility::wstring_view_t const utf16string& -> utf16string_view potential client fallout when enabling option: For clients passing char* to conversion that does not require conversion, string_view will be the result. If string type is actually required then an explicit ctor would need to be added. Otherwise client may be able to use more string_view in own code and futher reduce memory copies. See searchfile.cpp in Samples for example. Use of foo.c_str() and &foo[0] on a potential string_view is now foo.data().
- forced on for VS 2017 - forced off for VS 2015 - default for all others
+ add respect for CMAKE_CXX_STANDARD when set
…nto jason-ha/basic_string_view_support
…nto jason-ha/basic_string_view_support
#else | ||
inline const utility::string_t& to_string_t(const std::string& s) { return s; } | ||
#if CPPREST_USE_STRING_VIEWS | ||
inline utility::string_view_t to_string_t(utility::nstring_view_t s) { return s; } |
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.
All overloads of to_string_t
should return a string object, not a string view, since the expectations of the function is to return a string object:
/// <param name="s">A single byte character UTF-8 string.</param>
If you sometimes return a string object and sometimes a string view it may lead to unexpected disasters, e.g.:
auto text = to_string_t(getSomeText());
If getSomeText()
returns a string object or a string view, you're OK.
But if it returns something convertible to string view, after that statement the returned temporary is destroyed, text
is dangling now and you're most certainly screwed.
addresses issue #1216
Add CPPREST_USE_STRING_VIEWS option to accept string_views as input
most string utility and json APIs are changed to support string_views
(deprecated APIs left alone)
primary argument replacements:
const utility::string_t& -> utility::string_view_t
const std::string& -> utility::nstring_view_t
const std::wstring& -> utility::wstring_view_t
const utf16string& -> utf16string_view
potential client fallout when enabling option:
For clients passing char* to conversion that does not require conversion, string_view will be the result. If string type is actually required then an explicit ctor would need to be added. Otherwise client may be able to use more string_view in own code and futher reduce memory copies. See searchfile.cpp in Samples for example.
Use of foo.c_str() and &foo[0] on a potential string_view is now foo.data().
This PR also includes changes from PR #1230
To see changes with that PR as baseline review jason-ha#3.