From e4d371b0262a8e4d3b83d78c1d1eadbb54dee217 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Mon, 24 Feb 2020 22:20:56 -0400 Subject: [PATCH] Fixed string_view c++17 compatibility string_view isn't supposed to be implicitly convertible to std::string and code would break compiling under c++17 (when our local string_view is simply a std::string_view typedef). --- lokimq/bt_serialize.cpp | 14 ++++++---- lokimq/bt_serialize.h | 7 +++-- lokimq/lokimq.cpp | 54 +++++++++++++++++++++----------------- lokimq/lokimq.h | 23 ++++++++-------- lokimq/string_view.h | 2 +- tests/test_batch.cpp | 4 +-- tests/test_commands.cpp | 2 +- tests/test_connect.cpp | 4 +-- tests/test_string_view.cpp | 6 +++++ 9 files changed, 67 insertions(+), 49 deletions(-) diff --git a/lokimq/bt_serialize.cpp b/lokimq/bt_serialize.cpp index 3c0d2cf..9b7c272 100644 --- a/lokimq/bt_serialize.cpp +++ b/lokimq/bt_serialize.cpp @@ -145,7 +145,7 @@ bt_list_consumer::bt_list_consumer(string_view data_) : data{std::move(data_)} { /// Attempt to parse the next value as a string (and advance just past it). Throws if the next /// value is not a string. -string_view bt_list_consumer::consume_string() { +string_view bt_list_consumer::consume_string_view() { if (data.empty()) throw bt_deserialize_invalid{"expected a string, but reached end of data"}; else if (!is_string()) @@ -156,10 +156,14 @@ string_view bt_list_consumer::consume_string() { return result; } +std::string bt_list_consumer::consume_string() { + return std::string{consume_string_view()}; +} + /// Consumes a value without returning it. void bt_list_consumer::skip_value() { if (is_string()) - consume_string(); + consume_string_view(); else if (is_integer()) detail::bt_deserialize_integer(data); else if (is_list()) @@ -188,7 +192,7 @@ string_view bt_list_consumer::consume_dict_data() { if (data.size() < 2 || !is_dict()) throw bt_deserialize_invalid_type{"next bt value is not a dict"}; data.remove_prefix(1); // Descent into the dict, consumer the "d" while (!is_finished()) { - consume_string(); // Key is always a string + consume_string_view(); // Key is always a string if (!data.empty()) skip_value(); if (data.empty()) @@ -210,7 +214,7 @@ bool bt_dict_consumer::consume_key() { return true; if (data.empty()) throw bt_deserialize_invalid_type{"expected a key or dict end, found end of string"}; if (data[0] == 'e') return false; - key_ = bt_list_consumer::consume_string(); + key_ = bt_list_consumer::consume_string_view(); if (data.empty() || data[0] == 'e') throw bt_deserialize_invalid{"dict key isn't followed by a value"}; return true; @@ -220,7 +224,7 @@ std::pair bt_dict_consumer::next_string() { if (!is_string()) throw bt_deserialize_invalid_type{"expected a string, but found "s + data.front()}; std::pair ret; - ret.second = bt_list_consumer::consume_string(); + ret.second = bt_list_consumer::consume_string_view(); ret.first = flush_key(); return ret; } diff --git a/lokimq/bt_serialize.h b/lokimq/bt_serialize.h index f4f7633..746afa3 100644 --- a/lokimq/bt_serialize.h +++ b/lokimq/bt_serialize.h @@ -86,6 +86,7 @@ class bt_dict; /// Recursive generic type that can fully represent everything valid for a BT serialization. using bt_value = mapbox::util::variant< std::string, + string_view, int64_t, mapbox::util::recursive_wrapper, mapbox::util::recursive_wrapper @@ -572,7 +573,8 @@ public: /// Attempt to parse the next value as a string (and advance just past it). Throws if the next /// value is not a string. - string_view consume_string(); + std::string consume_string(); + string_view consume_string_view(); /// Attempts to parse the next value as an integer (and advance just past it). Throws if the /// next value is not an integer. @@ -798,7 +800,8 @@ public: /// value = d.consume_string(); /// - auto consume_string() { return next_string().second; } + auto consume_string_view() { return next_string().second; } + auto consume_string() { return std::string{consume_string_view()}; } template auto consume_integer() { return next_integer().second; } diff --git a/lokimq/lokimq.cpp b/lokimq/lokimq.cpp index 1f90740..ceb626b 100644 --- a/lokimq/lokimq.cpp +++ b/lokimq/lokimq.cpp @@ -119,13 +119,17 @@ std::vector as_strings(const MessageContainer& msgs) { return result; } -// Returns a string view of the given message data. If real std::string_views are available, -// returns one, otherwise returns a simple partial implementation of string_view. It's the caller's -// responsibility to keep the referenced message alive. -string_view view(const zmq::message_t &m) { +// Returns a string view of the given message data. It's the caller's responsibility to keep the +// referenced message alive. +string_view view(const zmq::message_t& m) { return {m.data(), m.size()}; } +// Like the above, but copies the data into a string. +std::string string(const zmq::message_t& m) { + return std::string{view(m)}; +} + // Builds a ZMTP metadata key-value pair. These will be available on every message from that peer. // Keys must start with X- and be <= 255 characters. std::string zmtp_metadata(string_view key, string_view value) { @@ -575,7 +579,8 @@ void LokiMQ::setup_outgoing_socket(zmq::socket_t& socket, string_view remote_pub } std::pair -LokiMQ::proxy_connect_sn(const std::string &remote, const std::string &connect_hint, bool optional, bool incoming_only, std::chrono::milliseconds keep_alive) { +LokiMQ::proxy_connect_sn(string_view remote_sv, string_view connect_hint, bool optional, bool incoming_only, std::chrono::milliseconds keep_alive) { + std::string remote{remote_sv}; auto &peer = peers[remote]; // We may auto-vivify here, but that's okay; it'll get cleaned up in idle_expiry if no connection gets established std::pair result = {nullptr, ""s}; @@ -614,7 +619,7 @@ LokiMQ::proxy_connect_sn(const std::string &remote, const std::string &connect_h // special inproc connection if self that doesn't need any external connection addr = SN_ADDR_SELF; } else { - addr = connect_hint; + addr = std::string{connect_hint}; if (addr.empty()) addr = peer_lookup(remote); else @@ -657,7 +662,7 @@ std::pair LokiMQ::proxy_connect_sn(bt_dict &&data) void LokiMQ::proxy_send(bt_dict_consumer data) { // NB: bt_dict_consumer goes in alphabetical order - std::string hint; + string_view hint; std::chrono::milliseconds keep_alive{DEFAULT_SEND_KEEP_ALIVE}; bool optional = false; bool incoming = false; @@ -665,7 +670,7 @@ void LokiMQ::proxy_send(bt_dict_consumer data) { std::string request_tag; std::unique_ptr request_cbptr; if (data.skip_until("hint")) - hint = data.consume_string(); + hint = data.consume_string_view(); if (data.skip_until("incoming")) incoming = data.consume_integer(); if (data.skip_until("keep-alive")) @@ -674,7 +679,7 @@ void LokiMQ::proxy_send(bt_dict_consumer data) { optional = data.consume_integer(); if (!data.skip_until("pubkey")) throw std::runtime_error("Internal error: Invalid proxy send command; pubkey missing"); - std::string remote_pubkey = data.consume_string(); + string_view remote_pubkey = data.consume_string_view(); if (data.skip_until("request")) request = data.consume_integer(); if (request) { @@ -710,7 +715,7 @@ void LokiMQ::proxy_send(bt_dict_consumer data) { if (e.num() == EHOSTUNREACH && sock_route.first == &listener && !sock_route.second.empty()) { // We *tried* to route via the incoming connection but it is no longer valid. Drop it, // establish a new connection, and try again. - auto &peer = peers[remote_pubkey]; + auto &peer = peers[std::string{remote_pubkey}]; peer.incoming.clear(); // Don't worry about cleaning the map entry if outgoing is also < 0: that will happen at the next idle cleanup LMQ_LOG(debug, "Could not route back to SN ", to_hex(remote_pubkey), " via listening socket; trying via new outgoing connection"); return proxy_send(std::move(data)); @@ -949,7 +954,7 @@ void LokiMQ::proxy_loop() { #ifndef NDEBUG if (log_level() >= LogLevel::trace) { - LMQ_TRACE("Reserving space for ", max_workers, " max workers = ", general_workers, " general + reserved:"); + LMQ_TRACE("Reserving space for ", max_workers, " max workers = ", general_workers, " general plus reservations for:"); for (const auto& cat : categories) LMQ_TRACE(" - ", cat.first, ": ", cat.second.reserved_threads); LMQ_TRACE(" - (batch jobs): ", batch_jobs_reserved); @@ -1106,7 +1111,7 @@ std::pair*> Lo LMQ_LOG(warn, "Invalid command '", command, "': expected ."); return {}; } - string_view catname{&command[0], dot}; + std::string catname = command.substr(0, dot); std::string cmd = command.substr(dot + 1); auto catit = categories.find(catname); @@ -1243,7 +1248,7 @@ bool LokiMQ::proxy_handle_builtin(int conn_index, std::vector& p LMQ_LOG(warn, "Received REPLY without a reply tag; ignoring"); return true; } - std::string reply_tag = view(parts[tag_pos]); + std::string reply_tag{view(parts[tag_pos])}; auto it = pending_requests.find(reply_tag); if (it != pending_requests.end()) { LMQ_LOG(debug, "Received REPLY for pending command", to_hex(reply_tag), "; scheduling callback"); @@ -1265,7 +1270,7 @@ bool LokiMQ::proxy_handle_builtin(int conn_index, std::vector& p return true; } LMQ_LOG(info, "Incoming client from ", peer_address(parts.back()), " sent HI, replying with HELLO"); - send_routed_message(listener, route, "HELLO"); + send_routed_message(listener, std::string{route}, "HELLO"); return true; } else if (cmd == "HELLO") { if (!is_outgoing_conn) { @@ -1394,7 +1399,7 @@ void LokiMQ::proxy_to_worker(size_t conn_index, std::vector& par if (is_outgoing_conn) send_direct_message(remotes[conn_index - listener.connected()].second, "UNKNOWNCOMMAND", command); else - send_routed_message(listener, pubkey, "UNKNOWNCOMMAND", command); + send_routed_message(listener, std::string{pubkey}, "UNKNOWNCOMMAND", command); return; } @@ -1419,7 +1424,7 @@ void LokiMQ::proxy_to_worker(size_t conn_index, std::vector& par } LMQ_LOG(debug, "No available free workers, queuing ", command, " for later"); - pending_commands.emplace_back(category, std::move(command), std::move(data_parts), cat_call.second, pubkey, peer_info.service_node); + pending_commands.emplace_back(category, std::move(command), std::move(data_parts), cat_call.second, std::string{pubkey}, peer_info.service_node); category.queued++; return; } @@ -1433,7 +1438,7 @@ void LokiMQ::proxy_to_worker(size_t conn_index, std::vector& par run.is_batch_job = false; run.cat = &category; run.command = std::move(command); - run.pubkey = pubkey; + run.pubkey = std::string{pubkey}; run.service_node = peer_info.service_node; run.data_parts = std::move(data_parts); run.callback = cat_call.second; @@ -1445,7 +1450,7 @@ void LokiMQ::proxy_to_worker(size_t conn_index, std::vector& par // it has changed (e.g. new connection). auto route = view(parts[0]); if (string_view(peer_info.incoming) != route) - peer_info.incoming = route; + peer_info.incoming = std::string{route}; } LMQ_TRACE("Forwarding incoming ", run.command, " from ", run.service_node ? "SN " : "non-SN ", @@ -1475,9 +1480,9 @@ bool LokiMQ::proxy_check_auth(string_view pubkey, size_t conn_index, const peer_ // issuing. Drop the connection; if the remote has something important to relay it will // reconnect, at which point we will reassess the SN status on the new incoming connection. if (!is_outgoing_conn) - send_routed_message(listener, pubkey, "BYE"); + send_routed_message(listener, std::string{pubkey}, "BYE"); else - proxy_disconnect(pubkey); + proxy_disconnect(std::string{pubkey}); return false; } @@ -1487,7 +1492,7 @@ bool LokiMQ::proxy_check_auth(string_view pubkey, size_t conn_index, const peer_ if (is_outgoing_conn) send_direct_message(remotes[conn_index - listener.connected()].second, std::move(reply), command); else - send_routed_message(listener, pubkey, std::move(reply), command); + send_routed_message(listener, std::string{pubkey}, std::move(reply), command); return true; } @@ -1537,7 +1542,7 @@ void LokiMQ::process_zap_requests(zmq::socket_t &zap_auth) { std::vector response_vals(6); response_vals[0] = "1.0"; // version if (frames.size() >= 2) - response_vals[1] = view(frames[1]); // unique identifier + response_vals[1] = std::string{view(frames[1])}; // unique identifier std::string &status_code = response_vals[2], &status_text = response_vals[3]; if (frames.size() < 6 || view(frames[0]) != "1.0") { @@ -1562,7 +1567,7 @@ void LokiMQ::process_zap_requests(zmq::socket_t &zap_auth) { status_code = "400"; status_text = "Unknown authentication domain: " + std::string{domain}; } else { - auto ip = view(frames[3]), pubkey = view(frames[6]); + auto ip = string(frames[3]), pubkey = string(frames[6]); auto result = allow_connection(ip, pubkey); bool sn = result.remote_sn; auto& user_id = response_vals[4]; @@ -1649,8 +1654,9 @@ void LokiMQ::proxy_connect_remote(bt_dict_consumer data) { remote_pubkey = data.consume_string(); assert(remote_pubkey.size() == 32 || remote_pubkey.empty()); } - if (data.skip_until("remote")) + if (data.skip_until("remote")) { remote = data.consume_string(); + } if (data.skip_until("timeout")) timeout = std::chrono::milliseconds{data.consume_integer()}; diff --git a/lokimq/lokimq.h b/lokimq/lokimq.h index ef70ba9..e1d38c4 100644 --- a/lokimq/lokimq.h +++ b/lokimq/lokimq.h @@ -115,7 +115,7 @@ public: /// If you want to send a non-strong reply even when the remote is a service node then add /// an explicit `send_option::optional()` argument. template - void send_back(const std::string& command, Args&&... args); + void send_back(string_view, Args&&... args); /// Sends a reply to a request. This takes no command: the command is always the built-in /// "REPLY" command, followed by the unique reply tag, then any reply data parts. All other @@ -204,12 +204,12 @@ public: /// /// @returns an `AuthLevel` enum value indicating the default auth level for the incoming /// connection, or AuthLevel::denied if the connection should be refused. - using AllowFunc = std::function; + using AllowFunc = std::function; /// Callback that is invoked when we need to send a "strong" message to a SN that we aren't /// already connected to and need to establish a connection. This callback returns the ZMQ /// connection string we should use which is typically a string such as `tcp://1.2.3.4:5678`. - using SNRemoteAddress = std::function; + using SNRemoteAddress = std::function; /// The callback type for registered commands. using CommandCallback = std::function; @@ -440,7 +440,7 @@ private: /// Common connection implementation used by proxy_connect/proxy_send. Returns the socket /// and, if a routing prefix is needed, the required prefix (or an empty string if not needed). /// For an optional connect that fail, returns nullptr for the socket. - std::pair proxy_connect_sn(const std::string& pubkey, const std::string& connect_hint, bool optional, bool incoming_only, std::chrono::milliseconds keep_alive); + std::pair proxy_connect_sn(string_view pubkey, string_view connect_hint, bool optional, bool incoming_only, std::chrono::milliseconds keep_alive); /// CONNECT_SN command telling us to connect to a new pubkey. Returns the socket (which could be /// existing or a new one). @@ -848,7 +848,7 @@ public: * performing a connection address lookup on the pubkey. */ template - void send(const std::string& pubkey, const std::string& cmd, const T&... opts); + void send(string_view pubkey, string_view cmd, const T&... opts); /** Send a command configured as a "REQUEST" command: the data parts will be prefixed with a * random identifier. The remote is expected to reply with a ["REPLY", , ...] @@ -862,8 +862,7 @@ public: * @param opts - anything else (i.e. strings, send_options) is forwarded to send(). */ template - void request(const std::string& pubkey, const std::string& cmd, ReplyCallback callback, - const T&... opts); + void request(string_view pubkey, string_view cmd, ReplyCallback callback, const T&... opts); /// The key pair this LokiMQ was created with; if empty keys were given during construction then /// this returns the generated keys. @@ -942,7 +941,7 @@ void send_control(zmq::socket_t& sock, string_view cmd, std::string data = {}); /// Base case: takes a string-like value and appends it to the message parts inline void apply_send_option(bt_list& parts, bt_dict&, string_view arg) { - parts.push_back(arg); + parts.emplace_back(arg); } /// `data_parts` specialization: appends a range of serialized data parts to the parts to send @@ -975,7 +974,7 @@ inline void apply_send_option(bt_list&, bt_dict& control_data, const send_option } // namespace detail template -void LokiMQ::send(const std::string& pubkey, const std::string& cmd, const T &...opts) { +void LokiMQ::send(string_view pubkey, string_view cmd, const T &...opts) { bt_dict control_data; bt_list parts{{cmd}}; #ifdef __cpp_fold_expressions @@ -992,7 +991,7 @@ void LokiMQ::send(const std::string& pubkey, const std::string& cmd, const T &.. std::string make_random_string(size_t size); template -void LokiMQ::request(const std::string& pubkey, const std::string& cmd, ReplyCallback callback, const T &...opts) { +void LokiMQ::request(string_view pubkey, string_view cmd, ReplyCallback callback, const T &...opts) { auto reply_tag = make_random_string(15); // 15 should keep us in most stl implementations' small string optimization bt_dict control_data; bt_list parts{{cmd, reply_tag}}; @@ -1011,7 +1010,7 @@ void LokiMQ::request(const std::string& pubkey, const std::string& cmd, ReplyCal } template -void Message::send_back(const std::string& command, Args&&... args) { +void Message::send_back(string_view command, Args&&... args) { assert(reply_tag.empty()); if (service_node) lokimq.send(pubkey, command, std::forward(args)...); else lokimq.send(pubkey, command, send_option::optional{}, std::forward(args)...); @@ -1034,7 +1033,7 @@ void LokiMQ::log_(LogLevel lvl, const char* file, int line, const T&... stuff) { std::ostringstream os; #ifdef __cpp_fold_expressions - os << ... << stuff; + (os << ... << stuff); #else (void) std::initializer_list{(os << stuff, 0)...}; #endif diff --git a/lokimq/string_view.h b/lokimq/string_view.h index e0efbe4..8671883 100644 --- a/lokimq/string_view.h +++ b/lokimq/string_view.h @@ -73,7 +73,7 @@ public: constexpr size_t length() const noexcept { return size_; } constexpr size_t max_size() const noexcept { return std::numeric_limits::max(); } constexpr bool empty() const noexcept { return size_ == 0; } - operator std::string() const { return {data_, size_}; } + explicit operator std::string() const { return {data_, size_}; } constexpr const char* begin() const noexcept { return data_; } constexpr const char* cbegin() const noexcept { return data_; } constexpr const char* end() const noexcept { return data_ + size_; } diff --git a/tests/test_batch.cpp b/tests/test_batch.cpp index 293d1ba..fbb3c07 100644 --- a/tests/test_batch.cpp +++ b/tests/test_batch.cpp @@ -45,7 +45,7 @@ TEST_CASE("batching many small jobs", "[batch-many]") { "", "", // generate ephemeral keys false, // not a service node {}, // don't listen - [](auto &) { return ""; }, + [](auto) { return ""; }, [](auto ip, auto pk) { return lokimq::Allow{lokimq::AuthLevel::none, false}; }, }; lmq.set_general_threads(4); @@ -63,7 +63,7 @@ TEST_CASE("batch exception propagation", "[batch-exceptions]") { "", "", // generate ephemeral keys false, // not a service node {}, // don't listen - [](auto &) { return ""; }, + [](auto) { return ""; }, [](auto ip, auto pk) { return lokimq::Allow{lokimq::AuthLevel::none, false}; }, }; lmq.set_general_threads(4); diff --git a/tests/test_commands.cpp b/tests/test_commands.cpp index 37d7533..f65acd7 100644 --- a/tests/test_commands.cpp +++ b/tests/test_commands.cpp @@ -25,7 +25,7 @@ TEST_CASE("basic commands", "[commands]") { }); std::string client_pubkey; server.add_command("public", "client.pubkey", [&](Message& m) { - client_pubkey = m.pubkey; + client_pubkey = std::string{m.pubkey}; }); server.start(); diff --git a/tests/test_connect.cpp b/tests/test_connect.cpp index 9c37eb7..1dbefd4 100644 --- a/tests/test_connect.cpp +++ b/tests/test_connect.cpp @@ -11,11 +11,11 @@ TEST_CASE("connections", "[connect][curve]") { "", "", // generate ephemeral keys false, // not a service node {listen}, - [](auto &) { return ""; }, + [](auto) { return ""; }, [](auto /*ip*/, auto /*pk*/) { return Allow{AuthLevel::none, false}; }, get_logger("S» ") }; -// server.log_level(LogLevel::trace); + server.log_level(LogLevel::trace); server.add_category("public", Access{AuthLevel::none}); server.add_request_command("public", "hello", [&](Message& m) { m.send_reply("hi"); }); diff --git a/tests/test_string_view.cpp b/tests/test_string_view.cpp index 74f30dc..dd87f35 100644 --- a/tests/test_string_view.cpp +++ b/tests/test_string_view.cpp @@ -170,8 +170,14 @@ TEST_CASE("string view", "[string_view]") { REQUIRE( string_view{"abc"} != string_view{"abcd"} ); REQUIRE( string_view{"abc"} < string_view{"abcd"} ); REQUIRE( string_view{"abc"} < string_view{"abd"} ); + REQUIRE_FALSE( string_view{"abd"} < string_view{"abc"} ); + REQUIRE_FALSE( string_view{"abcd"} < string_view{"abc"} ); + REQUIRE_FALSE( string_view{"abc"} < string_view{"abc"} ); REQUIRE( string_view{"abd"} > string_view{"abc"} ); REQUIRE( string_view{"abcd"} > string_view{"abc"} ); + REQUIRE_FALSE( string_view{"abc"} > string_view{"abd"} ); + REQUIRE_FALSE( string_view{"abc"} > string_view{"abcd"} ); + REQUIRE_FALSE( string_view{"abc"} > string_view{"abc"} ); REQUIRE( string_view{"abc"} <= string_view{"abcd"} ); REQUIRE( string_view{"abc"} <= string_view{"abc"} ); REQUIRE( string_view{"abc"} <= string_view{"abd"} );