Modernize cmake scripts and support MSVC 2017 - #834
Conversation
Sorry, something went wrong.
Visual Studio 2017 cannot decide if it is boost::integral_constant<bool,true> boost::true_type or boost::spirit::true_type cpp-netlib@a5252b9
This option allows us to setup _WIN32_WINNT definition value from cmake command line Default 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)
deanberris
left a comment
There was a problem hiding this comment.
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?
Sorry, something went wrong.
| 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.
Please re-base, as I've just merged a change that addresses this particular issue.
Sorry, something went wrong.
| *(+((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.
Can you separate out this change, (or rebase) to its own pull request?
Sorry, something went wrong.
| #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.
Is this strictly necessary? If so, can you make this a self-contained PR?
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
anonimal#1