do some long-standing TODOs

src/Channel.cc

@@ -241,13 +241,21 @@ Channel::Message Channel::recv(bool print_contents) {
   };
 }
 
-void Channel::send(uint16_t cmd, uint32_t flag, const void* data, size_t size,
-    bool print_contents) {
+void Channel::send(uint16_t cmd, uint32_t flag, bool print_contents) {
+  this->send(cmd, flag, nullptr, 0, print_contents);
+}
+
+void Channel::send(uint16_t cmd, uint32_t flag, const std::vector<std::pair<const void*, size_t>> blocks, bool print_contents) {
   if (!this->connected()) {
     channel_exceptions_log.warning("Attempted to send command on closed channel; dropping data");
     return;
   }
 
+  size_t size = 0;
+  for (const auto& b : blocks) {
+    size += b.second;
+  }
+
   string send_data;
   size_t logical_size;
   size_t send_data_size = 0;
@@ -317,10 +325,11 @@ void Channel::send(uint16_t cmd, uint32_t flag, const void* data, size_t size,
     throw runtime_error("outbound command too large");
   }
 
-  if (send_data.size() < send_data_size) {
-    send_data.append(reinterpret_cast<const char*>(data), size);
-    send_data.resize(send_data_size, '\0');
+  send_data.reserve(send_data_size);
+  for (const auto& b : blocks) {
+    send_data.append(reinterpret_cast<const char*>(b.first), b.second);
   }
+  send_data.resize(send_data_size, '\0');
 
   if (print_contents && (this->terminal_send_color != TerminalFormat::END)) {
     if (use_terminal_colors && this->terminal_send_color != TerminalFormat::NORMAL) {
@@ -347,6 +356,11 @@ void Channel::send(uint16_t cmd, uint32_t flag, const void* data, size_t size,
   evbuffer_add(buf, send_data.data(), send_data.size());
 }
 
+void Channel::send(
+    uint16_t cmd, uint32_t flag, const void* data, size_t size, bool print_contents) {
+  this->send(cmd, flag, {make_pair(data, size)}, print_contents);
+}
+
 void Channel::send(uint16_t cmd, uint32_t flag, const string& data, bool print_contents) {
   this->send(cmd, flag, data.data(), data.size(), print_contents);
 }

src/Channel.hh

@@ -78,7 +78,9 @@ struct Channel {
   Message recv(bool print_contents = true);
 
   // Sends a message with an automatically-constructed header.
-  void send(uint16_t cmd, uint32_t flag = 0, const void* data = nullptr, size_t size = 0, bool print_contents = true);
+  void send(uint16_t cmd, uint32_t flag = 0, bool print_contents = true);
+  void send(uint16_t cmd, uint32_t flag, const void* data, size_t size, bool print_contents = true);
+  void send(uint16_t cmd, uint32_t flag, const std::vector<std::pair<const void*, size_t>> blocks, bool print_contents = true);
   void send(uint16_t cmd, uint32_t flag, const std::string& data, bool print_contents = true);
   template <typename CmdT>
   void send(uint16_t cmd, uint32_t flag, const CmdT& data) {

src/ChatCommands.cc

@@ -815,13 +815,11 @@ static void server_command_edit(shared_ptr<ServerState> s, shared_ptr<Lobby> l,
   s->send_lobby_join_notifications(l, c);
 }
 
-// TODO: implement this
-// TODO: make sure the bank name is filesystem-safe
+// TODO: implement this (and make sure the bank name is filesystem-safe)
 /* static void server_command_change_bank(shared_ptr<ServerState>, shared_ptr<Lobby>,
     shared_ptr<Client> c, const std::u16string&) {
   check_version(c, GameVersion::BB);
-
-  TODO
+  ...
 } */
 
 // TODO: This can be implemented on the proxy server too.
@@ -960,8 +958,6 @@ static void server_command_ban(shared_ptr<ServerState> s, shared_ptr<Lobby> l,
     usecs *= 60 * 60 * 24 * 365;
   }
 
-  // TODO: put the length of time in this message. or don't; presumably the
-  // person deserved it
   s->license_manager->ban_until(target->license->serial_number, now() + usecs);
   send_message_box(target, u"$C6You were banned by a moderator.");
   target->should_disconnect = true;

src/Episode3/DataIndex.cc

@@ -1717,8 +1717,6 @@ uint64_t DataIndex::card_definitions_mtime() const {
 
 const string& DataIndex::get_compressed_map_list() const {
   if (this->compressed_map_list.empty()) {
-    // TODO: Write a version of prs_compress that takes iovecs (or something
-    // similar) so we can eliminate all this string copying here.
     StringWriter entries_w;
     StringWriter strings_w;
 

src/Main.cc

@@ -716,8 +716,6 @@ int main(int argc, char** argv) {
       } else if (!seed.empty()) {
         round1_seed = stoul(seed, nullptr, 16);
       } else {
-        // TODO: We can support brute-forcing character file encryption, but I'm
-        // lazy and this would probably not be useful for anyone.
         throw runtime_error("either --sys or --seed must be given");
       }
 

src/ReceiveCommands.cc

@@ -34,10 +34,6 @@ const char* ADD_NEXT_CLIENT_DISCONNECT_HOOK_NAME = "add_next_game_client";
 static shared_ptr<const Menu> proxy_options_menu_for_client(
     shared_ptr<ServerState> s, shared_ptr<const Client> c) {
   shared_ptr<Menu> ret(new Menu(MenuID::PROXY_OPTIONS, u"Proxy options"));
-  // Note: The descriptions are instead in the map above, because this menu is
-  // dynamically created every time it's sent to the client. This is just one
-  // way in which the menu abstraction is currently insufficient (there is a
-  // TODO about this in README.md).
   ret->items.emplace_back(ProxyOptionsMenuItemID::GO_BACK, u"Go back", u"Return to the\nProxy Server menu", 0);
 
   auto add_option = [&](uint32_t item_id, bool is_enabled, const char16_t* text, const char16_t* description) -> void {
@@ -291,9 +287,6 @@ void on_disconnect(shared_ptr<ServerState> s, shared_ptr<Client> c) {
     s->remove_client_from_lobby(c);
   }
 
-  // TODO: Track play time somewhere for BB players
-  // c->game_data.player()->disp.play_time += ((now() - c->play_time_begin) / 1000000);
-
   // Note: The client's GameData destructor should save their player data
   // shortly after this point
 }
@@ -2925,7 +2918,6 @@ static void on_00E2_BB(shared_ptr<ServerState>, shared_ptr<Client> c,
   auto& cmd = check_size_t<KeyAndTeamConfigBB>(data,
       sizeof(KeyAndTeamConfigBB) - 4, sizeof(KeyAndTeamConfigBB));
   c->game_data.account()->key_config = cmd;
-  // TODO: We should probably send a response here, but I don't know which one!
 }
 
 static void on_89(shared_ptr<ServerState> s, shared_ptr<Client> c,

src/ReceiveSubcommands.cc

@@ -236,12 +236,10 @@ static void on_sync_joining_player_item_state(shared_ptr<ServerState>,
         c->log.info("Byteswapped and recompressed item sync data (%zX bytes)", out_compressed_data.size());
       }
 
-      // TODO: It'd be nice to not copy the data so many times here.
-      StringWriter out_w;
-      out_w.put<G_SyncGameStateHeader_6x6B_6x6C_6x6D_6x6E>(out_cmd);
-      out_w.write(out_compressed_data);
-
-      send_command(target, command, flag, out_w.str());
+      vector<pair<const void*, size_t>> blocks;
+      blocks.emplace_back(make_pair(&out_cmd, sizeof(out_cmd)));
+      blocks.emplace_back(make_pair(out_compressed_data.data(), out_compressed_data.size()));
+      send_command(target, command, flag, blocks);
     }
   }
 }
@@ -1010,8 +1008,6 @@ static void on_equip_unequip_item(shared_ptr<ServerState>,
     throw logic_error("item tracking not enabled in BB game");
   }
 
-  // TODO: Should we forward this command on BB? The old version of newserv
-  // didn't, but that seems wrong.
   forward_subcommand(l, c, command, flag, data, size);
 }
 
@@ -1607,14 +1603,7 @@ static void on_identify_item_bb(shared_ptr<ServerState>,
     c->game_data.player()->disp.meseta -= 100;
     c->game_data.identify_result = c->game_data.player()->inventory.items[x];
     c->game_data.identify_result.data.data1[4] &= 0x7F;
-
-    // TODO: move this into a SendCommands.cc function
-    G_IdentifyResult_BB_6xB9 res;
-    res.header.subcommand = 0xB9;
-    res.header.size = sizeof(res) / 4;
-    res.header.client_id = c->lobby_client_id;
-    res.item_data = c->game_data.identify_result.data;
-    send_command_t(l, 0x60, 0x00, res);
+    send_item_identify_result(l, c);
 
   } else {
     forward_subcommand(l, c, command, flag, data, size);

src/SendCommands.cc

@@ -58,6 +58,11 @@ const unordered_set<string> bb_crypt_initial_client_commands({
     string("\xDC\x00\xDB\x00\x00\x00\x00\x00", 8),
 });
 
+void send_command(std::shared_ptr<Client> c, uint16_t command,
+    uint32_t flag, const std::vector<std::pair<const void*, size_t>>& blocks) {
+  c->channel.send(command, flag, blocks);
+}
+
 void send_command(shared_ptr<Client> c, uint16_t command, uint32_t flag,
     const void* data, size_t size) {
   c->channel.send(command, flag, data, size);
@@ -617,20 +622,14 @@ void send_patch_file(shared_ptr<Client> c, shared_ptr<PatchFileIndex::File> f) {
   send_command_t(c, 0x06, 0x00, open_cmd);
 
   for (size_t x = 0; x < f->chunk_crcs.size(); x++) {
-    // TODO: The use of StringWriter here is... unfortunate. Write a version of
-    // Channel::send that takes iovecs or something to avoid these dumb massive
-    // string copies.
-    StringWriter w;
     auto data = f->load_data();
     size_t chunk_size = min<uint32_t>(f->size - (x * 0x4000), 0x4000);
-    S_WriteFileHeader_Patch_07 write_cmd_header = {
-        x, f->chunk_crcs[x], chunk_size};
-    w.put(write_cmd_header);
-    w.write(data->data() + (x * 0x4000), chunk_size);
-    while (w.size() & 7) {
-      w.put_u8(0);
-    }
-    send_command(c, 0x07, 0x00, w.str());
+
+    vector<pair<const void*, size_t>> blocks;
+    S_WriteFileHeader_Patch_07 cmd_header = {x, f->chunk_crcs[x], chunk_size};
+    blocks.emplace_back(&cmd_header, sizeof(cmd_header));
+    blocks.emplace_back(data->data() + (x * 0x4000), chunk_size);
+    send_command(c, 0x07, 0x00, blocks);
   }
 
   S_CloseCurrentFile_Patch_08 close_cmd = {0};
@@ -2000,6 +1999,18 @@ void send_destroy_item(shared_ptr<Lobby> l, shared_ptr<Client> c,
   send_command_t(l, 0x60, 0x00, cmd);
 }
 
+void send_item_identify_result(shared_ptr<Lobby> l, shared_ptr<Client> c) {
+  if (c->version() != GameVersion::BB) {
+    throw logic_error("cannot send item identify result to non-BB client");
+  }
+  G_IdentifyResult_BB_6xB9 res;
+  res.header.subcommand = 0xB9;
+  res.header.size = sizeof(res) / 4;
+  res.header.client_id = c->lobby_client_id;
+  res.item_data = c->game_data.identify_result.data;
+  send_command_t(l, 0x60, 0x00, res);
+}
+
 void send_bank(shared_ptr<Client> c) {
   if (c->version() != GameVersion::BB) {
     throw logic_error("6xBC can only be sent to BB clients");
@@ -2522,7 +2533,7 @@ void send_ep3_update_spectator_count(shared_ptr<Lobby> l) {
   // this function just to check a behavior flag)
 
   // Note: We can't use send_command_t(l, ...) here because that would send the
-  // same command to l and to all wather lobbies. The commands should have
+  // same command to l and to all watcher lobbies. The commands should have
   // different values depending on who's in each watcher lobby, so we have to
   // manually send to each client here.
   for (auto c : l->clients) {

src/SendCommands.hh

@@ -32,6 +32,9 @@ extern const std::unordered_set<std::string> bb_crypt_initial_client_commands;
 //   pointer is given but size is accidentally not given (e.g. if the type of
 //   data in the calling function is changed from string to void*).
 
+void send_command(std::shared_ptr<Client> c, uint16_t command,
+    uint32_t flag, const std::vector<std::pair<const void*, size_t>>& blocks);
+
 void send_command(std::shared_ptr<Client> c, uint16_t command,
     uint32_t flag, const void* data, size_t size);
 
@@ -313,6 +316,7 @@ void send_create_inventory_item(std::shared_ptr<Lobby> l, std::shared_ptr<Client
     const ItemData& item);
 void send_destroy_item(std::shared_ptr<Lobby> l, std::shared_ptr<Client> c,
     uint32_t item_id, uint32_t amount);
+void send_item_identify_result(std::shared_ptr<Lobby> l, std::shared_ptr<Client> c);
 void send_bank(std::shared_ptr<Client> c);
 void send_shop(std::shared_ptr<Client> c, uint8_t shop_type);
 void send_level_up(std::shared_ptr<Lobby> l, std::shared_ptr<Client> c);

src/Text.hh

@@ -354,12 +354,6 @@ struct parray {
   }
 } __attribute__((packed));
 
-// TODO: It appears that these actually do not have to be null-terminated in PSO
-// commands some of the time. As an example, creating a game with a name with
-// the maximum length results in a C1 command with no null byte between the game
-// name and the password. We should be able to handle this by making ptexts not
-// required to be null-terminated in storage - this will still be safe if we
-// limit all operations by Count.
 template <typename CharT, size_t Count>
 struct ptext : parray<CharT, Count> {
   ptext() {