[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX updated this revision to Diff 278230.
ArcsinX added a comment.

Update comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83621/new/

https://reviews.llvm.org/D83621

Files:
  clang/lib/Tooling/FileMatchTrie.cpp
  clang/unittests/Tooling/CompilationDatabaseTest.cpp


Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===================================================================
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -281,6 +281,15 @@
   EXPECT_EQ("Cannot resolve relative paths", Error);
 }
 
+TEST_F(FileMatchTrieTest, SingleFile) {
+  Trie.insert("/root/RootFile.cc");
+  EXPECT_EQ("", find("/root/rootfile.cc"));
+  // Add subpath to avoid `if (Children.empty())` special case
+  // which we hit at previous `find()`.
+  Trie.insert("/root/otherpath/OtherFile.cc");
+  EXPECT_EQ("", find("/root/rootfile.cc"));
+}
+
 TEST(findCompileArgsInJsonDatabase, FindsNothingIfEmpty) {
   std::string ErrorMessage;
   CompileCommand NotFound = findCompileArgsInJsonDatabase(
Index: clang/lib/Tooling/FileMatchTrie.cpp
===================================================================
--- clang/lib/Tooling/FileMatchTrie.cpp
+++ clang/lib/Tooling/FileMatchTrie.cpp
@@ -105,8 +105,13 @@
                            StringRef FileName,
                            bool &IsAmbiguous,
                            unsigned ConsumedLength = 0) const {
+    // Note: we support only directory symlinks for performance reasons.
     if (Children.empty()) {
-      if (Comparator.equivalent(StringRef(Path), FileName))
+      // As far as we do not support file symlinks, compare
+      // basenames here to avoid request to file system.
+      if (llvm::sys::path::filename(Path) ==
+              llvm::sys::path::filename(FileName) &&
+          Comparator.equivalent(StringRef(Path), FileName))
         return StringRef(Path);
       return {};
     }
@@ -121,6 +126,13 @@
       if (!Result.empty() || IsAmbiguous)
         return Result;
     }
+
+    // If `ConsumedLength` is zero, this is the root and we have no filename
+    // match. Give up in this case, we don't try to find symlinks with
+    // different names.
+    if (ConsumedLength == 0)
+      return {};
+
     std::vector<StringRef> AllChildren;
     getAll(AllChildren, MatchingChild);
     StringRef Result;



Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===================================================================
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -281,6 +281,15 @@
   EXPECT_EQ("Cannot resolve relative paths", Error);
 }
 
+TEST_F(FileMatchTrieTest, SingleFile) {
+  Trie.insert("/root/RootFile.cc");
+  EXPECT_EQ("", find("/root/rootfile.cc"));
+  // Add subpath to avoid `if (Children.empty())` special case
+  // which we hit at previous `find()`.
+  Trie.insert("/root/otherpath/OtherFile.cc");
+  EXPECT_EQ("", find("/root/rootfile.cc"));
+}
+
 TEST(findCompileArgsInJsonDatabase, FindsNothingIfEmpty) {
   std::string ErrorMessage;
   CompileCommand NotFound = findCompileArgsInJsonDatabase(
Index: clang/lib/Tooling/FileMatchTrie.cpp
===================================================================
--- clang/lib/Tooling/FileMatchTrie.cpp
+++ clang/lib/Tooling/FileMatchTrie.cpp
@@ -105,8 +105,13 @@
                            StringRef FileName,
                            bool &IsAmbiguous,
                            unsigned ConsumedLength = 0) const {
+    // Note: we support only directory symlinks for performance reasons.
     if (Children.empty()) {
-      if (Comparator.equivalent(StringRef(Path), FileName))
+      // As far as we do not support file symlinks, compare
+      // basenames here to avoid request to file system.
+      if (llvm::sys::path::filename(Path) ==
+              llvm::sys::path::filename(FileName) &&
+          Comparator.equivalent(StringRef(Path), FileName))
         return StringRef(Path);
       return {};
     }
@@ -121,6 +126,13 @@
       if (!Result.empty() || IsAmbiguous)
         return Result;
     }
+
+    // If `ConsumedLength` is zero, this is the root and we have no filename
+    // match. Give up in this case, we don't try to find symlinks with
+    // different names.
+    if (ConsumedLength == 0)
+      return {};
+
     std::vector<StringRef> AllChildren;
     getAll(AllChildren, MatchingChild);
     StringRef Result;

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits