sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Just nits
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:359
if (Entry) {
- auto PublicHeader = CanonIncludes.mapHeader(Entry->getName());
+ auto PublicHeader = CanonIncludes.mapHeader(Entry->getLastRef());
if (!PublicHeader.empty()) {
----------------
getLastRef().getName() is just the same as Entry->getName().
We should be using SM.getFileEntryRefForID instead.
(Which is actually still not preserving the FERef under the hood, but will
soon...)
================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:40
+ /// Adds a file-to-string mapping from \p ID to \p CanonicalPath.
+ void addMapping(const FileEntryRef Header, llvm::StringRef CanonicalPath);
----------------
no const, FileEntryRef is passed by value (and below)
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:384
if (Includes) {
- llvm::StringRef Canonical = Includes->mapHeader(Filename);
+ llvm::StringRef Canonical = Includes->mapHeader(FE->getLastRef());
if (!Canonical.empty()) {
----------------
again, avoid getLastRef
================
Comment at: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp:26
+FileEntryRef
+addFile(llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> &FS,
+ FileManager &FM, llvm::StringRef Filename) {
----------------
no need to take ownership, just take InMemoryFileSystem&?
================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1593
+ CanonicalIncludes Includes;
+ Includes.addMapping((*File)->getLastRef(), "<canonical>");
+ CollectorOpts.CollectIncludePath = true;
----------------
avoid getLastRef
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123031/new/
https://reviews.llvm.org/D123031
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
