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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Hmm, there is actually a case the old behavior may have been papering over: case-insensitive filesystems. If the real file is foo.cc and we query Foo.cc, then the trie would (inefficiently) return the correct file.
It may be a good idea to make the trie case-insensitive for this purpose, but maybe we should wait until someone complains. That's a different patch, anyway.



================
Comment at: clang/lib/Tooling/FileMatchTrie.cpp:111
+      // As far as we do not support file symlinks we compare
+      // basenames here to avoid expensive request to file system.
+      if (llvm::sys::path::filename(Path) ==
----------------
Here it's really just for consistency - we have a single candidate, and calling equivalent() on a single file isn't expensive. I'd be happy with or without this check, but the comment should mention consistency.


================
Comment at: clang/lib/Tooling/FileMatchTrie.cpp:131
+    // We failed to find the file with `Children.find()`.
+    // If `ConsumedLength` is equal to 0 then we have tried to find the file
+    // with it's basename. Thus, we do not have files with the same
----------------
nit: This is quite a lot of words to come to "we do not support file symlinks" - I think it could be a bit shorter and also get into motivation:

"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".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83621