[lldb] [Utility/UriParser] Replace port==-1 with llvm::None

[mgorny]

Summary

Use llvm::Optional<uint16_t> instead of int for port number
in UriParser::Parse(), and use llvm::None to indicate missing port
instead of a magic value of -1.

[labath]

Seems like a (slight) improvement. You don't have to do this, but what I think would best help here is if we made a struct for these parsed results:

struct URI {
  static Optional<URI> parse(StringRef);
  StringRef scheme;
  ...
}

throw in an operator== and << and even the unit tests would become nicer.

lldb/include/lldb/Utility/UriParser.h

 //===-- UriParser.h ---------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UTILITY_URIPARSER_H
 #define LLDB_UTILITY_URIPARSER_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 
 namespace lldb_private {
 class UriParser {
 public:
   // Parses
   // RETURN VALUE
   //   if url is valid, function returns true and
   //   scheme/hostname/port/path are set to the parsed values
-  //   port it set to -1 if it is not included in the URL
+  //   port it set to llvm::None if it is not included in the URL
   //
   //   if the url is invalid, function returns false and
   //   output parameters remain unchanged
   static bool Parse(llvm::StringRef uri, llvm::StringRef &scheme,
-                    llvm::StringRef &hostname, int &port,
+                    llvm::StringRef &hostname, llvm::Optional<uint16_t> &port,
                     llvm::StringRef &path);
 };
 }
 
 #endif // LLDB_UTILITY_URIPARSER_H

lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp

Status PlatformAndroid::ConnectRemote(Args &args) {
   m_device_id.clear();
 
   if (IsHost())
     return Status("can't connect to the host platform, always connected");
 
   if (!m_remote_platform_sp)
     m_remote_platform_sp = PlatformSP(new PlatformAndroidRemoteGDBServer());
 
-  int port;
+  llvm::Optional<uint16_t> port;
   llvm::StringRef scheme, host, path;
   const char *url = args.GetArgumentAtIndex(0);
   if (!url)
     return Status("URL is null.");
   if (!UriParser::Parse(url, scheme, host, port, path))
     return Status("Invalid URL: %s", url);
   if (host != "localhost")
     m_device_id = std::string(host);

lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

 
 Status PlatformAndroidRemoteGDBServer::ConnectRemote(Args &args) {
   m_device_id.clear();
 
   if (args.GetArgumentCount() != 1)
     return Status(
         "\"platform connect\" takes a single argument: <connect-url>");
 
-  int remote_port;
+  llvm::Optional<uint16_t> remote_port;
   llvm::StringRef scheme, host, path;
   const char *url = args.GetArgumentAtIndex(0);
   if (!url)
     return Status("URL is null.");
   if (!UriParser::Parse(url, scheme, host, remote_port, path))
     return Status("Invalid URL: %s", url);
   if (host != "localhost")
     m_device_id = std::string(host);
 
   m_socket_namespace.reset();
   if (scheme == "unix-connect")
     m_socket_namespace = AdbClient::UnixSocketNamespaceFileSystem;
   else if (scheme == "unix-abstract-connect")
     m_socket_namespace = AdbClient::UnixSocketNamespaceAbstract;
 
   std::string connect_url;
-  auto error =
-      MakeConnectURL(g_remote_platform_pid, (remote_port < 0) ? 0 : remote_port,
+  auto error = MakeConnectURL(g_remote_platform_pid, remote_port.getValueOr(0),
                               path, connect_url);
 
   if (error.Fail())
     return error;
 
   args.ReplaceArgumentAtIndex(0, connect_url);
 
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
   LLDB_LOGF(log, "Rewritten platform connect URL: %s", connect_url.c_str());
lldb::ProcessSP PlatformAndroidRemoteGDBServer::ConnectProcess(
     lldb_private::Debugger &debugger, lldb_private::Target *target,
     lldb_private::Status &error) {
   // We don't have the pid of the remote gdbserver when it isn't started by us
   // but we still want to store the list of port forwards we set up in our port
   // forward map. Generate a fake pid for these cases what won't collide with
   // any other valid pid on android.
   static lldb::pid_t s_remote_gdbserver_fake_pid = 0xffffffffffffffffULL;
 
-  int remote_port;
+  llvm::Optional<uint16_t> remote_port;
   llvm::StringRef scheme, host, path;
   if (!UriParser::Parse(connect_url, scheme, host, remote_port, path)) {
     error.SetErrorStringWithFormat("Invalid URL: %s",
                                    connect_url.str().c_str());
     return nullptr;
   }
 
   std::string new_connect_url;
   error = MakeConnectURL(s_remote_gdbserver_fake_pid--,
-                         (remote_port < 0) ? 0 : remote_port, path,
-                         new_connect_url);
+                         remote_port.getValueOr(0), path, new_connect_url);
   if (error.Fail())
     return nullptr;
 
   return PlatformRemoteGDBServer::ConnectProcess(new_connect_url, plugin_name,
                                                  debugger, target, error);
 }

lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp

error.SetErrorString(
         "\"platform connect\" takes a single argument: <connect-url>");
     return error;
   }
 
   const char *url = args.GetArgumentAtIndex(0);
   if (!url)
     return Status("URL is null.");
 
-  int port;
+  llvm::Optional<uint16_t> port;
   llvm::StringRef scheme, hostname, pathname;
   if (!UriParser::Parse(url, scheme, hostname, port, pathname))
     return Status("Invalid URL: %s", url);
 
   // We're going to reuse the hostname when we connect to the debugserver.
   m_platform_scheme = std::string(scheme);
   m_platform_hostname = std::string(hostname);
 

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/GDBRemote.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/UnimplementedError.h"
-#include "lldb/Utility/UriParser.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 #include "ProcessGDBRemote.h"
 #include "ProcessGDBRemoteLog.h"
 #include "lldb/Utility/StringExtractorGDBRemote.h"
 

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp

 // debugserver does not accept the URL scheme prefix.
 #if !defined(__APPLE__)
   url << m_socket_scheme << "://";
 #endif
   uint16_t *port_ptr = port.getPointer();
   if (m_socket_protocol == Socket::ProtocolTcp) {
     llvm::StringRef platform_scheme;
     llvm::StringRef platform_ip;
-    int platform_port;
+    llvm::Optional<uint16_t> platform_port;
     llvm::StringRef platform_path;
     std::string platform_uri = GetConnection()->GetURI();
     bool ok = UriParser::Parse(platform_uri, platform_scheme, platform_ip,
                                platform_port, platform_path);
     UNUSED_IF_ASSERT_DISABLED(ok);
     assert(ok);
     url << '[' << platform_ip.str() << "]:" << *port;
   } else {

lldb/source/Utility/UriParser.cpp

 
 #include <cstdint>
 #include <tuple>
 
 using namespace lldb_private;
 
 // UriParser::Parse
 bool UriParser::Parse(llvm::StringRef uri, llvm::StringRef &scheme,
-                      llvm::StringRef &hostname, int &port,
+                      llvm::StringRef &hostname, llvm::Optional<uint16_t> &port,
                       llvm::StringRef &path) {
   llvm::StringRef tmp_scheme, tmp_hostname, tmp_path;
 
   const llvm::StringRef kSchemeSep("://");
   auto pos = uri.find(kSchemeSep);
   if (pos == std::string::npos)
     return false;
 
bool UriParser::Parse(llvm::StringRef uri, llvm::StringRef &scheme,
 
   // Extract port
   if (!host_port.empty()) {
     uint16_t port_value = 0;
     if (host_port.getAsInteger(0, port_value))
       return false;
     port = port_value;
   } else
-    port = -1;
+    port = llvm::None;
 
   scheme = tmp_scheme;
   hostname = tmp_hostname;
   path = tmp_path;
   return true;
 }

lldb/tools/lldb-server/Acceptor.cpp

 std::string Acceptor::GetLocalSocketId() const { return m_local_socket_id(); }
 
 std::unique_ptr<Acceptor> Acceptor::Create(StringRef name,
                                            const bool child_processes_inherit,
                                            Status &error) {
   error.Clear();
 
   Socket::SocketProtocol socket_protocol = Socket::ProtocolUnixDomain;
-  int port;
+  llvm::Optional<uint16_t> port;
   StringRef scheme, host, path;
   // Try to match socket name as URL - e.g., tcp://localhost:5555
   if (UriParser::Parse(name, scheme, host, port, path)) {
     if (!FindProtocolByScheme(scheme.str().c_str(), socket_protocol))
       error.SetErrorStringWithFormat("Unknown protocol scheme \"%s\"",
                                      scheme.str().c_str());
     else
       name = name.drop_front(scheme.size() + strlen("://"));

lldb/unittests/Host/ConnectionFileDescriptorTest.cpp

void TestGetURI(std::string ip) {
     std::unique_ptr<TCPSocket> socket_a_up;
     std::unique_ptr<TCPSocket> socket_b_up;
     CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up);
     auto socket = socket_a_up.release();
     ConnectionFileDescriptor connection_file_descriptor(socket);
 
     llvm::StringRef scheme;
     llvm::StringRef hostname;
-    int port;
+    llvm::Optional<uint16_t> port;
     llvm::StringRef path;
     std::string uri(connection_file_descriptor.GetURI());
     EXPECT_TRUE(UriParser::Parse(uri, scheme, hostname, port, path));
     EXPECT_EQ(ip, hostname);
-    EXPECT_EQ(socket->GetRemotePortNumber(), port);
+    EXPECT_EQ(socket->GetRemotePortNumber(), port.getValue());
   }
 };
 
 TEST_F(ConnectionFileDescriptorTest, TCPGetURIv4) {
   if (!HostSupportsIPv4())
     return;
   TestGetURI("127.0.0.1");
 }
 
 TEST_F(ConnectionFileDescriptorTest, TCPGetURIv6) {
   if (!HostSupportsIPv6())
     return;
   TestGetURI("::1");
 }

lldb/unittests/Host/SocketTest.cpp

TEST_P(SocketTest, TCPGetConnectURI) {
   std::unique_ptr<TCPSocket> socket_b_up;
   if (!HostSupportsProtocol())
     return;
   CreateTCPConnectedSockets(GetParam().localhost_ip, &socket_a_up,
                             &socket_b_up);
 
   llvm::StringRef scheme;
   llvm::StringRef hostname;
-  int port;
+  llvm::Optional<uint16_t> port;
   llvm::StringRef path;
   std::string uri(socket_a_up->GetRemoteConnectionURI());
   EXPECT_TRUE(UriParser::Parse(uri, scheme, hostname, port, path));
   EXPECT_EQ(scheme, "connect");
   EXPECT_EQ(port, socket_a_up->GetRemotePortNumber());
 }
 
 TEST_P(SocketTest, UDPGetConnectURI) {
   // UDPSocket::Connect() creates sockets with AF_INET (IPv4).
   if (!HostSupportsIPv4())
     return;
   llvm::Expected<std::unique_ptr<UDPSocket>> socket =
       UDPSocket::Connect("127.0.0.1:0", /*child_processes_inherit=*/false);
   ASSERT_THAT_EXPECTED(socket, llvm::Succeeded());
 
   llvm::StringRef scheme;
   llvm::StringRef hostname;
-  int port;
+  llvm::Optional<uint16_t> port;
   llvm::StringRef path;
   std::string uri = socket.get()->GetRemoteConnectionURI();
   EXPECT_TRUE(UriParser::Parse(uri, scheme, hostname, port, path));
   EXPECT_EQ(scheme, "udp");
 }
 
 #if LLDB_ENABLE_POSIX
 TEST_P(SocketTest, DomainGetConnectURI) {
   llvm::SmallString<64> domain_path;
   std::error_code EC =
       llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", domain_path);
   ASSERT_FALSE(EC);
   llvm::sys::path::append(domain_path, "test");
 
   // Skip the test if the $TMPDIR is too long to hold a domain socket.
   if (domain_path.size() > 107u)
     return;
 
   std::unique_ptr<DomainSocket> socket_a_up;
   std::unique_ptr<DomainSocket> socket_b_up;
   CreateDomainConnectedSockets(domain_path, &socket_a_up, &socket_b_up);
 
   llvm::StringRef scheme;
   llvm::StringRef hostname;
-  int port;
+  llvm::Optional<uint16_t> port;
   llvm::StringRef path;
   std::string uri(socket_a_up->GetRemoteConnectionURI());
   EXPECT_TRUE(UriParser::Parse(uri, scheme, hostname, port, path));
   EXPECT_EQ(scheme, "unix-connect");
   EXPECT_EQ(path, domain_path);
 
   EXPECT_EQ(socket_b_up->GetRemoteConnectionURI(), "");
 }

lldb/unittests/Utility/UriParserTest.cpp

 #include "lldb/Utility/UriParser.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;
 
 // result strings (scheme/hostname/port/path) passed into UriParser::Parse
 // are initialized to kAsdf so we can verify that they are unmodified if the
 // URI is invalid
 static const char *kAsdf = "asdf";
 
 class UriTestCase {
 public:
   UriTestCase(const char *uri, const char *scheme, const char *hostname,
-              int port, const char *path)
+              llvm::Optional<uint16_t> port, const char *path)
       : m_uri(uri), m_result(true), m_scheme(scheme), m_hostname(hostname),
         m_port(port), m_path(path) {}
 
   UriTestCase(const char *uri)
       : m_uri(uri), m_result(false), m_scheme(kAsdf), m_hostname(kAsdf),
         m_port(1138), m_path(kAsdf) {}
 
   const char *m_uri;
   bool m_result;
   const char *m_scheme;
   const char *m_hostname;
-  int m_port;
+  llvm::Optional<uint16_t> m_port;
   const char *m_path;
 };
 
 #define VALIDATE                                                               \
   llvm::StringRef scheme(kAsdf);                                               \
   llvm::StringRef hostname(kAsdf);                                             \
-  int port(1138);                                                              \
+  llvm::Optional<uint16_t> port(1138);                                         \
   llvm::StringRef path(kAsdf);                                                 \
   EXPECT_EQ(testCase.m_result,                                                 \
             UriParser::Parse(testCase.m_uri, scheme, hostname, port, path));   \
   EXPECT_STREQ(testCase.m_scheme, scheme.str().c_str());                       \
   EXPECT_STREQ(testCase.m_hostname, hostname.str().c_str());                   \
   EXPECT_EQ(testCase.m_port, port);                                            \
   EXPECT_STREQ(testCase.m_path, path.str().c_str());
 
 TEST(UriParserTest, Minimal) {
-  const UriTestCase testCase("x://y", "x", "y", -1, "/");
+  const UriTestCase testCase("x://y", "x", "y", llvm::None, "/");
   VALIDATE
 }
 
 TEST(UriParserTest, MinimalPort) {
   const UriTestCase testCase("x://y:1", "x", "y", 1, "/");
   llvm::StringRef scheme(kAsdf);
   llvm::StringRef hostname(kAsdf);
-  int port(1138);
+  llvm::Optional<uint16_t> port(1138);
   llvm::StringRef path(kAsdf);
   bool result = UriParser::Parse(testCase.m_uri, scheme, hostname, port, path);
   EXPECT_EQ(testCase.m_result, result);
 
   EXPECT_STREQ(testCase.m_scheme, scheme.str().c_str());
   EXPECT_STREQ(testCase.m_hostname, hostname.str().c_str());
   EXPECT_EQ(testCase.m_port, port);
   EXPECT_STREQ(testCase.m_path, path.str().c_str());
 }
 
 TEST(UriParserTest, MinimalPath) {
-  const UriTestCase testCase("x://y/", "x", "y", -1, "/");
+  const UriTestCase testCase("x://y/", "x", "y", llvm::None, "/");
   VALIDATE
 }
 
 TEST(UriParserTest, MinimalPortPath) {
   const UriTestCase testCase("x://y:1/", "x", "y", 1, "/");
   VALIDATE
 }
 
 TEST(UriParserTest, LongPath) {
-  const UriTestCase testCase("x://y/abc/def/xyz", "x", "y", -1, "/abc/def/xyz");
+  const UriTestCase testCase("x://y/abc/def/xyz", "x", "y", llvm::None,
+                             "/abc/def/xyz");
   VALIDATE
 }
 
 TEST(UriParserTest, TypicalPortPathIPv4) {
   const UriTestCase testCase("connect://192.168.100.132:5432/", "connect",
                              "192.168.100.132", 5432, "/");
   VALIDATE;
 }
 
 TEST(UriParserTest, TypicalPortPathIPv6) {
   const UriTestCase testCase(
       "connect://[2601:600:107f:db64:a42b:4faa:284:3082]:5432/", "connect",
       "2601:600:107f:db64:a42b:4faa:284:3082", 5432, "/");
   VALIDATE;
 }
 
 TEST(UriParserTest, BracketedHostnamePort) {
   const UriTestCase testCase("connect://[192.168.100.132]:5432/", "connect",
                              "192.168.100.132", 5432, "/");
   llvm::StringRef scheme(kAsdf);
   llvm::StringRef hostname(kAsdf);
-  int port(1138);
+  llvm::Optional<uint16_t> port(1138);
   llvm::StringRef path(kAsdf);
   bool result = UriParser::Parse(testCase.m_uri, scheme, hostname, port, path);
   EXPECT_EQ(testCase.m_result, result);
 
   EXPECT_STREQ(testCase.m_scheme, scheme.str().c_str());
   EXPECT_STREQ(testCase.m_hostname, hostname.str().c_str());
   EXPECT_EQ(testCase.m_port, port);
   EXPECT_STREQ(testCase.m_path, path.str().c_str());
 }
 
 TEST(UriParserTest, BracketedHostname) {
   const UriTestCase testCase("connect://[192.168.100.132]", "connect",
-                             "192.168.100.132", -1, "/");
+                             "192.168.100.132", llvm::None, "/");
   VALIDATE
 }
 
 TEST(UriParserTest, BracketedHostnameWithPortIPv4) {
   // Android device over IPv4: port is a part of the hostname.
   const UriTestCase testCase("connect://[192.168.100.132:1234]", "connect",
-                             "192.168.100.132:1234", -1, "/");
+                             "192.168.100.132:1234", llvm::None, "/");
   VALIDATE
 }
 
 TEST(UriParserTest, BracketedHostnameWithPortIPv6) {
   // Android device over IPv6: port is a part of the hostname.
   const UriTestCase testCase(
       "connect://[[2601:600:107f:db64:a42b:4faa:284]:1234]", "connect",
-      "[2601:600:107f:db64:a42b:4faa:284]:1234", -1, "/");
+      "[2601:600:107f:db64:a42b:4faa:284]:1234", llvm::None, "/");
   VALIDATE
 }
 
 TEST(UriParserTest, BracketedHostnameWithColon) {
   const UriTestCase testCase("connect://[192.168.100.132:5555]:1234", "connect",
                              "192.168.100.132:5555", 1234, "/");
   VALIDATE
 }