Add logging when an AbsolutePath instance is not actually absolute

jacobdufault · Apr 19, 2018 · af8997e · af8997e
1 parent 2ed3437
commit af8997e
Show file tree

Hide file tree

Showing 9 changed files with 107 additions and 97 deletions.
diff --git a/src/file_types.cc b/src/file_types.cc
@@ -3,15 +3,17 @@
 #include "platform.h"
 #include "utils.h"
 
-// static
-AbsolutePath AbsolutePath::Build(const std::string& path) {
-  // TODO: add a debug-only check here that this is actually normalized.
-  return AbsolutePath(path);
-}
+#include <loguru.hpp>
 
 AbsolutePath::AbsolutePath() {}
 
-AbsolutePath::AbsolutePath(const std::string& path) : path(path) {}
+AbsolutePath::AbsolutePath(const std::string& path, bool validate)
+    : path(path) {
+  if (validate && !IsAbsolutePath(path)) {
+    loguru::Text stack = loguru::stacktrace();
+    LOG_S(ERROR) << "Expected " << path << " to be absolute\n" << stack.c_str();
+  }
+}
 
 AbsolutePath::operator std::string() const {
   return path;

diff --git a/src/file_types.h b/src/file_types.h
@@ -1,25 +1,24 @@
 #pragma once
 
+#include "hash_utils.h"
 #include "serializer.h"
 
 #include <string>
 
 struct AbsolutePath {
-  // Use platform.h NormalizePath instead of calling this.
-  static AbsolutePath Build(const std::string& path);
-
   // Try not to use this.
   AbsolutePath();
 
   // Provide implicit conversions to std::string for the time being.
-  AbsolutePath(const std::string& path);
+  AbsolutePath(const std::string& path, bool validate = true);
   operator std::string() const;
 
   bool operator==(const AbsolutePath& rhs) const;
   bool operator!=(const AbsolutePath& rhs) const;
 
   std::string path;
 };
+MAKE_HASHABLE(AbsolutePath, t.path);
 
 void Reflect(Reader& visitor, AbsolutePath& value);
 void Reflect(Writer& visitor, AbsolutePath& value);
@@ -31,4 +30,5 @@ struct Directory {
   bool operator!=(const Directory& rhs) const;
 
   std::string path;
-};
+};
+MAKE_HASHABLE(Directory, t.path);
diff --git a/src/hash_utils.h b/src/hash_utils.h
@@ -0,0 +1,43 @@
+#pragma once
+
+#include <functional>
+
+// http://stackoverflow.com/a/38140932
+//
+//  struct SomeHashKey {
+//    std::string key1;
+//    std::string key2;
+//    bool key3;
+//  };
+//  MAKE_HASHABLE(SomeHashKey, t.key1, t.key2, t.key3)
+
+inline void hash_combine(std::size_t& seed) {}
+
+template <typename T, typename... Rest>
+inline void hash_combine(std::size_t& seed, const T& v, Rest... rest) {
+  std::hash<T> hasher;
+  seed ^= hasher(v) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
+  hash_combine(seed, rest...);
+}
+
+#define MAKE_HASHABLE(type, ...)                  \
+  namespace std {                                 \
+  template <>                                     \
+  struct hash<type> {                             \
+    std::size_t operator()(const type& t) const { \
+      std::size_t ret = 0;                        \
+      hash_combine(ret, __VA_ARGS__);             \
+      return ret;                                 \
+    }                                             \
+  };                                              \
+  }
+
+#define MAKE_ENUM_HASHABLE(type)                  \
+  namespace std {                                 \
+  template <>                                     \
+  struct hash<type> {                             \
+    std::size_t operator()(const type& t) const { \
+      return hash<int>()(static_cast<int>(t));    \
+    }                                             \
+  };                                              \
+  }
diff --git a/src/import_pipeline.cc b/src/import_pipeline.cc
@@ -829,16 +829,16 @@ TEST_SUITE("ImportPipeline") {
       std::unique_ptr<IndexFile> opt_previous_index;
       if (!old_args.empty()) {
         opt_previous_index =
-            std::make_unique<IndexFile>(AbsolutePath("---.cc"), "<empty>");
+            std::make_unique<IndexFile>(AbsolutePath("---.cc", false /*validate*/), "<empty>");
         opt_previous_index->args = old_args;
       }
       optional<AbsolutePath> from;
       if (is_dependency)
-        from = AbsolutePath("---.cc");
+        from = AbsolutePath("---.cc", false /*validate*/);
       return FileNeedsParse(
           is_interactive /*is_interactive*/, &timestamp_manager,
           &modification_timestamp_fetcher, &import_manager, cache_manager,
-          opt_previous_index.get(), AbsolutePath(file), new_args, from);
+          opt_previous_index.get(), AbsolutePath(file, false /*validate*/), new_args, from);
     };
 
     // A file with no timestamp is not imported, since this implies the file no

diff --git a/src/platform_posix.cc b/src/platform_posix.cc
@@ -107,7 +107,7 @@ optional<AbsolutePath> RealPathNotExpandSymlink(std::string path,
   // Remove trailing slash except when a single "/".
   if (resolved.size() > 1 && resolved.back() == '/')
     resolved.pop_back();
-  return AbsolutePath(resolved);
+  return AbsolutePath(resolved, true /*validate*/);
 }
 
 }  // namespace

diff --git a/src/platform_win.cc b/src/platform_win.cc
@@ -102,7 +102,7 @@ optional<AbsolutePath> NormalizePath(const std::string& path0,
   // cquery assumes forward-slashes.
   std::replace(path.begin(), path.end(), '\\', '/');
 
-  return AbsolutePath(path);
+  return AbsolutePath(path, false /*validate*/);
 }
 
 bool TryMakeDirectory(const AbsolutePath& absolute_path) {

diff --git a/src/project.cc b/src/project.cc
@@ -69,19 +69,6 @@ struct NormalizationCache {
   }
 };
 
-bool IsUnixAbsolutePath(const std::string& path) {
-  return !path.empty() && path[0] == '/';
-}
-
-bool IsWindowsAbsolutePath(const std::string& path) {
-  auto is_drive_letter = [](char c) {
-    return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');
-  };
-
-  return path.size() > 3 && path[1] == ':' &&
-         (path[2] == '/' || path[2] == '\\') && is_drive_letter(path[0]);
-}
-
 enum class ProjectMode { CompileCommandsJson, DotCquery, ExternalCommand };
 
 struct ProjectConfig {
@@ -154,8 +141,7 @@ const std::vector<std::string>& GetSystemIncludes(
 
   std::vector<std::string> compiler_drivers = {
       GetExecutablePathNextToCqueryBinary("cquery-clang"), "clang++", "g++"};
-  if (IsUnixAbsolutePath(compiler_driver) ||
-      IsWindowsAbsolutePath(compiler_driver)) {
+  if (IsAbsolutePath(compiler_driver)) {
     compiler_drivers.insert(compiler_drivers.begin(), compiler_driver);
   }
 
@@ -229,8 +215,7 @@ Project::Entry GetCompilationEntryFromCompileCommandEntry(
     // TODO/FIXME: Normalization will fail for paths that do not exist. Should
     // it return an optional<std::string>?
     assert(!path.empty());
-    if (entry.directory.empty() || IsUnixAbsolutePath(path) ||
-        IsWindowsAbsolutePath(path)) {
+    if (entry.directory.empty() || IsAbsolutePath(path)) {
       // We still want to normalize, as the path may contain .. characters.
       return config->normalization_cache.Get(path);
     }
@@ -538,7 +523,7 @@ std::vector<Project::Entry> LoadCompilationEntriesFromDirectory(
 
   comp_db_dir = opt_compilation_db_dir;
 
-  if (!IsUnixAbsolutePath(comp_db_dir) && !IsWindowsAbsolutePath(comp_db_dir)) {
+  if (!IsAbsolutePath(comp_db_dir)) {
     comp_db_dir =
         project->normalization_cache.Get(project->project_dir + comp_db_dir);
   }
@@ -609,11 +594,11 @@ std::vector<Project::Entry> LoadCompilationEntriesFromDirectory(
     our_time.Resume();
     entry.directory = directory;
     std::string absolute_filename;
-    if (IsUnixAbsolutePath(relative_filename) ||
-        IsWindowsAbsolutePath(relative_filename))
+    if (IsAbsolutePath(relative_filename)) {
       absolute_filename = relative_filename;
-    else
+    } else {
       absolute_filename = directory + "/" + relative_filename;
+    }
     entry.file = project->normalization_cache.Get(absolute_filename);
 
     result.push_back(
@@ -1684,23 +1669,6 @@ TEST_SUITE("Project") {
     }
   }
 
-  TEST_CASE("IsWindowsAbsolutePath works correctly") {
-    REQUIRE(IsWindowsAbsolutePath("C:/Users/projects/"));
-    REQUIRE(IsWindowsAbsolutePath("C:/Users/projects"));
-    REQUIRE(IsWindowsAbsolutePath("C:/Users/projects"));
-    REQUIRE(IsWindowsAbsolutePath("C:\\Users\\projects"));
-    REQUIRE(IsWindowsAbsolutePath("C:\\\\Users\\\\projects"));
-    REQUIRE(IsWindowsAbsolutePath("c:\\\\Users\\\\projects"));
-    REQUIRE(IsWindowsAbsolutePath("A:\\\\Users\\\\projects"));
-
-    REQUIRE(!IsWindowsAbsolutePath("C:/"));
-    REQUIRE(!IsWindowsAbsolutePath("../abc/test"));
-    REQUIRE(!IsWindowsAbsolutePath("5:/test"));
-    REQUIRE(!IsWindowsAbsolutePath("cquery/project/file.cc"));
-    REQUIRE(!IsWindowsAbsolutePath(""));
-    REQUIRE(!IsWindowsAbsolutePath("/etc/linux/path"));
-  }
-
   TEST_CASE("Entry inference prefers same file endings") {
     Project p;
     {

diff --git a/src/utils.cc b/src/utils.cc
@@ -463,6 +463,42 @@ AbsolutePath GetExecutablePathNextToCqueryBinary(const std::string& name) {
   return AbsolutePath(executable_path.substr(0, pos + 1) + name);
 }
 
+bool IsAbsolutePath(const std::string& path) {
+  return IsUnixAbsolutePath(path) || IsWindowsAbsolutePath(path);
+}
+
+bool IsUnixAbsolutePath(const std::string& path) {
+  return !path.empty() && path[0] == '/';
+}
+
+bool IsWindowsAbsolutePath(const std::string& path) {
+  auto is_drive_letter = [](char c) {
+    return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');
+  };
+
+  return path.size() > 3 && path[1] == ':' &&
+         (path[2] == '/' || path[2] == '\\') && is_drive_letter(path[0]);
+}
+
+TEST_SUITE("AbsolutePath") {
+  TEST_CASE("IsWindowsAbsolutePath works correctly") {
+    REQUIRE(IsWindowsAbsolutePath("C:/Users/projects/"));
+    REQUIRE(IsWindowsAbsolutePath("C:/Users/projects"));
+    REQUIRE(IsWindowsAbsolutePath("C:/Users/projects"));
+    REQUIRE(IsWindowsAbsolutePath("C:\\Users\\projects"));
+    REQUIRE(IsWindowsAbsolutePath("C:\\\\Users\\\\projects"));
+    REQUIRE(IsWindowsAbsolutePath("c:\\\\Users\\\\projects"));
+    REQUIRE(IsWindowsAbsolutePath("A:\\\\Users\\\\projects"));
+
+    REQUIRE(!IsWindowsAbsolutePath("C:/"));
+    REQUIRE(!IsWindowsAbsolutePath("../abc/test"));
+    REQUIRE(!IsWindowsAbsolutePath("5:/test"));
+    REQUIRE(!IsWindowsAbsolutePath("cquery/project/file.cc"));
+    REQUIRE(!IsWindowsAbsolutePath(""));
+    REQUIRE(!IsWindowsAbsolutePath("/etc/linux/path"));
+  }
+}
+
 TEST_SUITE("GetDirName") {
   TEST_CASE("all") {
     REQUIRE(GetDirName("") == "./");

diff --git a/src/utils.h b/src/utils.h
@@ -125,46 +125,6 @@ void AddRange(std::vector<T>* dest, std::vector<T>&& to_add) {
                std::make_move_iterator(to_add.end()));
 }
 
-// http://stackoverflow.com/a/38140932
-//
-//  struct SomeHashKey {
-//    std::string key1;
-//    std::string key2;
-//    bool key3;
-//  };
-//  MAKE_HASHABLE(SomeHashKey, t.key1, t.key2, t.key3)
-
-inline void hash_combine(std::size_t& seed) {}
-
-template <typename T, typename... Rest>
-inline void hash_combine(std::size_t& seed, const T& v, Rest... rest) {
-  std::hash<T> hasher;
-  seed ^= hasher(v) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
-  hash_combine(seed, rest...);
-}
-
-#define MAKE_HASHABLE(type, ...)                  \
-  namespace std {                                 \
-  template <>                                     \
-  struct hash<type> {                             \
-    std::size_t operator()(const type& t) const { \
-      std::size_t ret = 0;                        \
-      hash_combine(ret, __VA_ARGS__);             \
-      return ret;                                 \
-    }                                             \
-  };                                              \
-  }
-
-#define MAKE_ENUM_HASHABLE(type)                  \
-  namespace std {                                 \
-  template <>                                     \
-  struct hash<type> {                             \
-    std::size_t operator()(const type& t) const { \
-      return hash<int>()(static_cast<int>(t));    \
-    }                                             \
-  };                                              \
-  }
-
 float GetProcessMemoryUsedInMb();
 
 std::string FormatMicroseconds(long long microseconds);
@@ -177,6 +137,7 @@ std::string UpdateToRnNewlines(std::string output);
 // Get an executable located next to the cquery binary.
 AbsolutePath GetExecutablePathNextToCqueryBinary(const std::string& name);
 
-// FIXME: These should go in file_types.h; move hashing into a separate header.
-MAKE_HASHABLE(AbsolutePath, t.path);
-MAKE_HASHABLE(Directory, t.path);
+// Utility methods to check if |path| is absolute.
+bool IsAbsolutePath(const std::string& path);
+bool IsUnixAbsolutePath(const std::string& path);
+bool IsWindowsAbsolutePath(const std::string& path);