Refactor canonical_path users; reduce std::string usage

Add append_component() and parent() to canonical_path so users of
canonical_path don't need to convert to strings manually.

This commit should not change behavior.

Author: strager

src/configuration-loader.cpp

@@ -63,9 +63,10 @@ configuration* configuration_loader::find_and_load_config_file(
     return nullptr;
   }
 
-  std::string parent_directory =
-      input_path ? parent_path(std::move(canonical_input_path).path())
-                 : std::move(canonical_input_path).path();
+  canonical_path parent_directory = std::move(canonical_input_path).canonical();
+  if (input_path) {
+    parent_directory.parent();
+  }
 
   // TODO(strager): Directory -> config to reduce lookups in cases like the
   // following:
@@ -74,14 +75,12 @@ configuration* configuration_loader::find_and_load_config_file(
   // config path: ./quick-lint-js.config
 
   for (;;) {
-    std::string config_path = parent_directory;
-    for (const std::string_view& suffix : {
-             QLJS_PREFERRED_PATH_DIRECTORY_SEPARATOR "quick-lint-js.config"sv,
-             QLJS_PREFERRED_PATH_DIRECTORY_SEPARATOR ".quick-lint-js.config"sv,
+    for (const std::string_view& file_name : {
+             "quick-lint-js.config"sv,
+             ".quick-lint-js.config"sv,
          }) {
-      config_path.resize(parent_directory.size());
-      QLJS_ASSERT(config_path == parent_directory);
-      config_path.append(suffix);
+      canonical_path config_path = parent_directory;
+      config_path.append_component(file_name);
 
       if (configuration* config = this->get_loaded_config(config_path)) {
         return config;
@@ -90,12 +89,11 @@ configuration* configuration_loader::find_and_load_config_file(
       read_file_result config_json = read_file(config_path.c_str());
       if (config_json.ok()) {
         auto [config_it, inserted] = this->loaded_config_files_.emplace(
-            std::piecewise_construct,
-            std::forward_as_tuple(canonical_path(std::string(config_path))),
+            std::piecewise_construct, std::forward_as_tuple(config_path),
             std::forward_as_tuple());
         QLJS_ASSERT(inserted);
         configuration* config = &config_it->second;
-        config->set_config_file_path(std::move(config_path));
+        config->set_config_file_path(std::move(config_path).path());
         config->load_from_json(&config_json.content);
         return config;
       }
@@ -108,33 +106,17 @@ configuration* configuration_loader::find_and_load_config_file(
     }
 
     // Loop, looking in parent directories.
-    std::string new_parent_directory =
-        parent_path(std::string(parent_directory));
-    if (new_parent_directory == parent_directory) {
+    if (!parent_directory.parent()) {
       // We searched the root directory which has no parent.
       break;
     }
-    parent_directory = std::move(new_parent_directory);
   }
 
   return &this->default_config_;
 }
 
 QLJS_WARNING_POP
 
-configuration* configuration_loader::get_loaded_config(
-    const std::string& path) noexcept {
-#if QLJS_HAVE_STD_TRANSPARENT_KEYS
-  auto existing_config_it = this->loaded_config_files_.find(path);
-#else
-  auto existing_config_it =
-      this->loaded_config_files_.find(canonical_path(std::string(path)));
-#endif
-  return existing_config_it == this->loaded_config_files_.end()
-             ? nullptr
-             : &existing_config_it->second;
-}
-
 configuration* configuration_loader::get_loaded_config(
     const canonical_path& path) noexcept {
   auto existing_config_it = this->loaded_config_files_.find(path);

src/file-canonical.cpp

@@ -44,6 +44,35 @@
 
 namespace quick_lint_js {
 namespace {
+#if QLJS_PATHS_WIN32
+using path_char = wchar_t;
+#elif QLJS_PATHS_POSIX
+using path_char = char;
+#else
+#error "Unsupported platform"
+#endif
+using path_string = std::basic_string<path_char>;
+using path_string_view = std::basic_string_view<path_char>;
+
+#if QLJS_PATHS_WIN32
+constexpr path_char preferred_component_separator = L'\\';
+constexpr path_char component_separators[] = L"/\\";
+constexpr path_string_view dot = L".";
+constexpr path_string_view dot_dot = L"..";
+#elif QLJS_PATHS_POSIX
+constexpr path_char preferred_component_separator = '/';
+constexpr path_char component_separators[] = "/";
+constexpr path_string_view dot = ".";
+constexpr path_string_view dot_dot = "..";
+#else
+#error "Unsupported platform"
+#endif
+
+// TODO(strager): Avoid hard-coding std::string. Use std::wstring always on
+// Windows.
+constexpr char preferred_component_separator_char =
+    static_cast<char>(preferred_component_separator);
+
 #if QLJS_HAVE_UNISTD_H
 int read_symbolic_link(const char *symlink_path, std::string *out);
 #endif
@@ -58,7 +87,9 @@ std::string string_for_error_message(std::wstring_view);
 #endif
 }
 
-canonical_path::canonical_path(std::string &&path) : path_(std::move(path)) {}
+canonical_path::canonical_path(std::string &&path) : path_(std::move(path)) {
+  QLJS_ASSERT(!this->path_.empty());
+}
 
 std::string_view canonical_path::path() const &noexcept { return this->path_; }
 
@@ -94,6 +125,50 @@ bool operator!=(const canonical_path &lhs, std::string_view rhs) noexcept {
   return !(lhs == rhs);
 }
 
+void canonical_path::append_component(std::string_view component) {
+  if (this->path_[this->path_.size() - 1] !=
+      preferred_component_separator_char) {
+    this->path_.push_back(preferred_component_separator_char);
+  }
+  this->path_.append(component);
+}
+
+bool canonical_path::parent() {
+#if QLJS_PATHS_POSIX
+  if (this->path_[this->path_.size() - 1] == '/') {
+    return false;
+  }
+  std::size_t slash_index = this->path_.find_last_of("/");
+  QLJS_ASSERT(slash_index != std::string::npos);
+  if (slash_index == 0 || slash_index == 1) {
+    // Preserve the slash for resulting root paths.
+    this->path_.resize(slash_index + 1);
+  } else {
+    this->path_.resize(slash_index);
+  }
+  return true;
+#elif QLJS_PATHS_WIN32
+  // TODO(strager): Avoid string conversions and copying.
+  std::optional<std::wstring> wpath = mbstring_to_wstring(this->path_.c_str());
+  QLJS_ASSERT(wpath.has_value());
+  HRESULT result = ::PathCchRemoveFileSpec(wpath->data(), wpath->size() + 1);
+  switch (result) {
+  case S_OK:
+    wpath->resize(std::wcslen(wpath->data()));
+    this->path_ = std::filesystem::path(*wpath).string();
+    return true;
+  case S_FALSE:
+    // Path is a root path already.
+    return false;
+  default:
+    QLJS_UNIMPLEMENTED();
+    break;
+  }
+#else
+#error "Unsupported platform"
+#endif
+}
+
 canonical_path_result::canonical_path_result() {}
 
 canonical_path_result::canonical_path_result(std::string &&path)
@@ -141,30 +216,6 @@ canonical_path_result canonical_path_result::failure(std::string &&error) {
 namespace {
 class path_canonicalizer {
  public:
-#if QLJS_PATHS_WIN32
-  using path_char = wchar_t;
-#elif QLJS_PATHS_POSIX
-  using path_char = char;
-#else
-#error "Unsupported platform"
-#endif
-  using path_string = std::basic_string<path_char>;
-  using path_string_view = std::basic_string_view<path_char>;
-
-#if QLJS_PATHS_WIN32
-  static constexpr path_char preferred_component_separator = L'\\';
-  static constexpr path_char component_separators[] = L"/\\";
-  static constexpr path_string_view dot = L".";
-  static constexpr path_string_view dot_dot = L"..";
-#elif QLJS_PATHS_POSIX
-  static constexpr path_char preferred_component_separator = '/';
-  static constexpr path_char component_separators[] = "/";
-  static constexpr path_string_view dot = ".";
-  static constexpr path_string_view dot_dot = "..";
-#else
-#error "Unsupported platform"
-#endif
-
   explicit path_canonicalizer(path_string_view path) : original_path_(path) {}
 
   canonical_path_result result() {

src/quick-lint-js/configuration-loader.h

@@ -22,7 +22,6 @@ class configuration_loader {
   configuration* load_config_file(const char* config_path);
   configuration* find_and_load_config_file(const char* input_path);
 
-  configuration* get_loaded_config(const std::string& path) noexcept;
   configuration* get_loaded_config(const canonical_path& path) noexcept;
 
   configuration default_config_;

src/quick-lint-js/file-canonical.h

@@ -30,6 +30,22 @@ class canonical_path {
   std::string &&path() && noexcept;
   const char *c_str() const noexcept;
 
+  // Add a new component to the end of the path.
+  //
+  // This function does not consult the filesystem.
+  void append_component(std::string_view component);
+
+  // Remove the last component of the path.
+  //
+  // If the path is a root path, this function returns false does not modify
+  // *this.
+  //
+  // If the path is not a root path, this function returns true and modifies
+  // *this.
+  //
+  // This function does not consult the filesystem.
+  bool parent();
+
   friend bool operator==(const canonical_path &,
                          const canonical_path &) noexcept;
   friend bool operator!=(const canonical_path &,

test/test-file-canonical.cpp

@@ -701,6 +701,107 @@ TEST_F(test_file_canonical, canonical_path_win32_root) {
   }
 }
 #endif
+
+#if defined(_POSIX_VERSION) && _POSIX_VERSION >= 200112L
+TEST_F(test_file_canonical, parent_of_root_is_root_posix) {
+  for (const char* root : {"/", "//"}) {
+    canonical_path p(root);
+    EXPECT_FALSE(p.parent());
+    EXPECT_EQ(p.path(), root);
+  }
+}
+
+TEST_F(test_file_canonical, parent_of_non_root_removes_component_posix) {
+  struct test_case {
+    const std::string_view before;
+    const std::string_view after;
+  };
+
+  for (const test_case& tc : {
+           test_case{"/hello", "/"},
+           test_case{"/dir/subdir", "/dir"},
+           test_case{"/a/b/c/d", "/a/b/c"},
+           test_case{"//implementation-defined", "//"},
+           test_case{"//implementation/defined", "//implementation"},
+       }) {
+    canonical_path p(std::string(tc.before));
+    EXPECT_TRUE(p.parent());
+    EXPECT_EQ(p.path(), tc.after) << "before = " << tc.before;
+  }
+}
+
+TEST_F(test_file_canonical, append_component_posix) {
+  struct test_case {
+    const std::string_view before;
+    const std::string_view component;
+    const std::string_view after;
+  };
+
+  for (const test_case& tc : {
+           test_case{"/", "hello", "/hello"},
+           test_case{"/dir", "subdir", "/dir/subdir"},
+           test_case{"//", "hello", "//hello"},
+           test_case{"//dir", "subdir", "//dir/subdir"},
+       }) {
+    canonical_path p(std::string(tc.before));
+    p.append_component(tc.component);
+    EXPECT_EQ(p.path(), tc.after)
+        << "before = " << tc.before << "\ncomponent = " << tc.component;
+  }
+}
+#endif
+
+#if defined(_WIN32)
+TEST_F(test_file_canonical, parent_of_root_is_root_win32) {
+  for (const char* root : {R"(C:\)", R"(\\?\X:\)", R"(\\server\share)"}) {
+    canonical_path p(root);
+    EXPECT_FALSE(p.parent());
+    EXPECT_EQ(p.path(), root);
+  }
+}
+
+TEST_F(test_file_canonical, parent_of_non_root_removes_component_win32) {
+  struct test_case {
+    const std::string_view before;
+    const std::string_view after;
+  };
+
+  for (const test_case& tc : {
+           test_case{R"(C:\hello)", R"(C:\)"},
+           test_case{R"(C:\dir\subdir)", R"(C:\dir)"},
+           test_case{R"(C:\a\b\c\d)", R"(C:\a\b\c)"},
+           test_case{R"(\\?\X:\hello)", R"(\\?\X:\)"},
+           test_case{R"(\\?\X:\dir\subdir)", R"(\\?\X:\dir)"},
+           test_case{R"(\\?\X:\a\b\c\d)", R"(\\?\X:\a\b\c)"},
+           test_case{R"(\\server\share\file)", R"(\\server\share)"},
+       }) {
+    canonical_path p(std::string(tc.before));
+    EXPECT_TRUE(p.parent());
+    EXPECT_EQ(p.path(), tc.after) << "before = " << tc.before;
+  }
+}
+
+TEST_F(test_file_canonical, append_component_win32) {
+  struct test_case {
+    const std::string_view before;
+    const std::string_view component;
+    const std::string_view after;
+  };
+
+  for (const test_case& tc : {
+           test_case{R"(C:\)", R"(hello)", R"(C:\hello)"},
+           test_case{R"(C:\dir)", R"(subdir)", R"(C:\dir\subdir)"},
+           test_case{R"(\\?\X:\)", R"(hello)", R"(\\?\X:\hello)"},
+           test_case{R"(\\?\X:\dir)", R"(subdir)", R"(\\?\X:\dir\subdir)"},
+           test_case{R"(\\server\share)", R"(dir)", R"(\\server\share\dir)"},
+       }) {
+    canonical_path p(std::string(tc.before));
+    p.append_component(tc.component);
+    EXPECT_EQ(p.path(), tc.after)
+        << "before = " << tc.before << "\ncomponent = " << tc.component;
+  }
+}
+#endif
 }
 }