[clangd] Expose actOnAllPArentDirectories helper

[kadircet]

Summary

Will be used in other components that need ancestor traversal.

[sammccall]

clang-tools-extra/clangd/support/Path.h

39

The signature is a bit weird here,

• prone to boolean-sense mistakes
• it's not obvious it only works on absolute paths
• the loop isn't hard to write

maybe we should expose absoluteParent instead?

[kadircet]

maybe we should expose absoluteParent instead?

doing that instead. also updating tidyprovider and configyamlparser to make use of this helper now, PTAL.

[kadircet]

• Expose absoluteParent instead of whole traverse action
• Use helper in existing places that use path::parent_path for traversal

[sammccall]

Thanks!

clang-tools-extra/clangd/ConfigProvider.cpp

100–101

absoluteParent?

I think this can be moved into the loop init now, it's not used anywhere else

clang-tools-extra/clangd/support/Path.h

37

Maybe add a hint why we use this...

"unlike sys::parent_path, does not consider C: a parent of C:\"

[kadircet]

• Update comment.
• Get rid of a parent_path usage.

[kadircet]

• Get rid of parent_path usage for real

clang-tools-extra/clangd/ConfigProvider.cpp

 //===--- ConfigProvider.cpp - Loading of user configuration ---------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "ConfigProvider.h"
 #include "Config.h"
 #include "ConfigFragment.h"
 #include "support/FileCache.h"
+#include "support/Path.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
 #include <chrono>
 #include <mutex>
 #include <string>
 
class RelFileProvider : public Provider {
 
     std::vector<CompiledFragment>
     getFragments(const Params &P, DiagnosticCallback DC) const override {
       namespace path = llvm::sys::path;
 
       if (P.Path.empty())
         return {};
 
       // Compute absolute paths to all ancestors (substrings of P.Path).
-      llvm::StringRef Parent = path::parent_path(P.Path);
       llvm::SmallVector<llvm::StringRef, 8> Ancestors;
-      for (auto I = path::begin(Parent, path::Style::posix),
-                E = path::end(Parent);
-           I != E; ++I) {
-        // Avoid weird non-substring cases like phantom "." components.
-        // In practice, Component is a substring for all "normal" ancestors.
-        if (I->end() < Parent.begin() || I->end() > Parent.end())
-          continue;
-        Ancestors.emplace_back(Parent.begin(), I->end() - Parent.begin());
+      for (auto Ancestor = absoluteParent(P.Path); !Ancestor.empty();
+           Ancestor = absoluteParent(Ancestor)) {
+        Ancestors.emplace_back(Ancestor);
       }
       // Ensure corresponding cache entries exist in the map.
       llvm::SmallVector<FileConfigCache *, 8> Caches;
       {
         std::lock_guard<std::mutex> Lock(Mu);
         for (llvm::StringRef Ancestor : Ancestors) {
           auto It = Cache.find(Ancestor);
           // Assemble the actual config file path only once.
           if (It == Cache.end()) {
             llvm::SmallString<256> ConfigPath = Ancestor;
             path::append(ConfigPath, RelPath);
             // Use native slashes for reading the file, affects diagnostics.
             llvm::sys::path::native(ConfigPath);
             It = Cache.try_emplace(Ancestor, ConfigPath.str(), Ancestor).first;
           }
           Caches.push_back(&It->second);
         }
       }
       // Finally query each individual file.
       // This will take a (per-file) lock for each file that actually exists.
       std::vector<CompiledFragment> Result;
-      for (FileConfigCache *Cache : Caches)
+      for (FileConfigCache *Cache : llvm::reverse(Caches))
         Cache->get(FS, DC, P.FreshTime, Result);
       return Result;
     };
 
   public:
     RelFileProvider(llvm::StringRef RelPath, const ThreadsafeFS &FS)
         : RelPath(RelPath), FS(FS) {
       assert(llvm::sys::path::is_relative(RelPath));

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

 #include <string>
 #include <tuple>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
-// Variant of parent_path that operates only on absolute paths.
-PathRef absoluteParent(PathRef Path) {
-  assert(llvm::sys::path::is_absolute(Path));
-#if defined(_WIN32)
-  // llvm::sys says "C:\" is absolute, and its parent is "C:" which is relative.
-  // This unhelpful behavior seems to have been inherited from boost.
-  if (llvm::sys::path::relative_path(Path).empty()) {
-    return PathRef();
-  }
-#endif
-  PathRef Result = llvm::sys::path::parent_path(Path);
-  assert(Result.empty() || llvm::sys::path::is_absolute(Result));
-  return Result;
-}
-
 // Runs the given action on all parent directories of filename, starting from
 // deepest directory and going up to root. Stops whenever action succeeds.
 void actOnAllParentDirectories(PathRef FileName,
                                llvm::function_ref<bool(PathRef)> Action) {
   for (auto Path = absoluteParent(FileName); !Path.empty() && !Action(Path);
        Path = absoluteParent(Path))
     ;
 }

clang-tools-extra/clangd/TidyProvider.cpp

 //===--- TidyProvider.cpp - create options for running clang-tidy----------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "TidyProvider.h"
 #include "../clang-tidy/ClangTidyModuleRegistry.h"
 #include "Config.h"
 #include "support/FileCache.h"
 #include "support/Logger.h"
+#include "support/Path.h"
 #include "support/ThreadsafeFS.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Process.h"
DotClangTidyTree(const ThreadsafeFS &FS)
       : FS(FS), RelPath(".clang-tidy"), MaxStaleness(std::chrono::seconds(5)) {}
 
   void apply(tidy::ClangTidyOptions &Result, PathRef AbsPath) {
     namespace path = llvm::sys::path;
     assert(path::is_absolute(AbsPath));
 
     // Compute absolute paths to all ancestors (substrings of P.Path).
     // Ensure cache entries for each ancestor exist in the map.
-    llvm::StringRef Parent = path::parent_path(AbsPath);
     llvm::SmallVector<DotClangTidyCache *> Caches;
     {
       std::lock_guard<std::mutex> Lock(Mu);
-      for (auto I = path::rbegin(Parent), E = path::rend(Parent); I != E; ++I) {
-        assert(I->end() >= Parent.begin() && I->end() <= Parent.end() &&
-               "Canonical path components should be substrings");
-        llvm::StringRef Ancestor(Parent.begin(), I->end() - Parent.begin());
-#ifdef _WIN32
-        // C:\ is an ancestor, but skip its (relative!) parent C:.
-        if (Ancestor.size() == 2 && Ancestor.back() == ':')
-          continue;
-#endif
-        assert(path::is_absolute(Ancestor));
-
+      for (auto Ancestor = absoluteParent(AbsPath); !Ancestor.empty();
+           Ancestor = absoluteParent(Ancestor)) {
         auto It = Cache.find(Ancestor);
-
         // Assemble the actual config file path only if needed.
         if (It == Cache.end()) {
           llvm::SmallString<256> ConfigPath = Ancestor;
           path::append(ConfigPath, RelPath);
           It = Cache.try_emplace(Ancestor, ConfigPath.str()).first;
         }
         Caches.push_back(&It->second);
       }

clang-tools-extra/clangd/support/Path.h

 using PathRef = llvm::StringRef;
 
 // For platforms where paths are case-insensitive (but case-preserving),
 // we need to do case-insensitive comparisons and use lowercase keys.
 // FIXME: Make Path a real class with desired semantics instead.
 std::string maybeCaseFoldPath(PathRef Path);
 bool pathEqual(PathRef, PathRef);
 
 /// Checks if \p Ancestor is a proper ancestor of \p Path. This is just a
 /// smarter lexical prefix match, e.g: foo/bar/baz doesn't start with foo/./bar.
 /// Both \p Ancestor and \p Path must be absolute.
 bool pathStartsWith(
     PathRef Ancestor, PathRef Path,
     llvm::sys::path::Style Style = llvm::sys::path::Style::native);
+
+/// Variant of parent_path that operates only on absolute paths.
+/// Unlike parent_path doesn't consider C: a parent of C:\.
+PathRef absoluteParent(PathRef Path);
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/support/Path.cpp

 #ifdef CLANGD_PATH_CASE_INSENSITIVE
 std::string maybeCaseFoldPath(PathRef Path) { return Path.lower(); }
 bool pathEqual(PathRef A, PathRef B) { return A.equals_lower(B); }
 #else  // NOT CLANGD_PATH_CASE_INSENSITIVE
 std::string maybeCaseFoldPath(PathRef Path) { return Path.str(); }
 bool pathEqual(PathRef A, PathRef B) { return A == B; }
 #endif // CLANGD_PATH_CASE_INSENSITIVE
 
+PathRef absoluteParent(PathRef Path) {
+  assert(llvm::sys::path::is_absolute(Path));
+#if defined(_WIN32)
+  // llvm::sys says "C:\" is absolute, and its parent is "C:" which is relative.
+  // This unhelpful behavior seems to have been inherited from boost.
+  if (llvm::sys::path::relative_path(Path).empty()) {
+    return PathRef();
+  }
+#endif
+  PathRef Result = llvm::sys::path::parent_path(Path);
+  assert(Result.empty() || llvm::sys::path::is_absolute(Result));
+  return Result;
+}
+
 bool pathStartsWith(PathRef Ancestor, PathRef Path,
                     llvm::sys::path::Style Style) {
   assert(llvm::sys::path::is_absolute(Ancestor) &&
          llvm::sys::path::is_absolute(Path));
   // If ancestor ends with a separator drop that, so that we can match /foo/ as
   // a parent of /foo.
   if (llvm::sys::path::is_separator(Ancestor.back(), Style))
     Ancestor = Ancestor.drop_back();