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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
