- Notifications
You must be signed in to change notification settings - Fork425
Modernize cmake scripts and support MSVC 2017#834
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:0.13-release
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Visual Studio 2017 cannot decide if it is boost::integral_constant<bool,true> boost::true_typeor boost::spirit::true_typecpp-netlib@a5252b9
This option allows us to setup _WIN32_WINNT definition valuefrom cmake command lineDefault value is 0x0501 (_WIN32_WINNT_WINXP)https://docs.microsoft.com/en-us/cpp/porting/modifying-winver-and-win32-winnt
48bbadb Build: Add cmake option CPP-NETLIB_WINAPI_VERSION (Tengiz Sharafiev)
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 love the idea of modernising the CMake config -- I think we should be more proactive in doing this going forward. However, I do have concerns about the scope of these changes.
Can you please break it up into smaller parts, separating the code changes from the CMake config changes?
| parse(partial_parsed.begin(), partial_parsed.end(), | ||
| (lit("HTTP/") >> ushort_ >> '.' >> ushort_), version_pair); | ||
| boost::spirit::qi::parse(partial_parsed.begin(), partial_parsed.end(), | ||
| (boost::spirit::qi::lit("HTTP/") >> boost::spirit::qi::ushort_ >> '.' >> boost::spirit::qi::ushort_), version_pair); |
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.
Please re-base, as I've just merged a change that addresses this particular issue.
| *(+((boost::spirit::qi::alnum |boost::spirit::qi::punct) - ':') >>boost::spirit::qi::lit(": ") >> | ||
| as_u32_string()[+((boost::spirit::qi::unicode::alnum |boost::spirit::qi::space |boost::spirit::qi::punct) - '\r' - '\n')] >> | ||
| boost::spirit::qi::lit("\r\n")) >> | ||
| boost::spirit::qi::lit("\r\n"), |
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 you separate out this change, (or rebase) to its own pull request?
| @@ -19,6 +19,7 @@ | |||
| #include <boost/asio/detail/throw_error.hpp> | |||
| #include <boost/asio/error.hpp> | |||
| #include <boost/asio/stream_socket_service.hpp> | |||
| #include <boost/asio/io_service.hpp> | |||
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.
Is this strictly necessary? If so, can you make this a self-contained PR?
Also, just to be clear, most of this already looks good to me -- I'm just a little concerned about the granularity of the changes. One of the failures in the travis CI suggests that there's one functional regression (in the tests for streaming get). It could be a transient error, so I'm re-running those to make sure it's not because of the changes in this PR. |
ping@btolfa |
anonimal#1