[clang] NFC: Remove `{File,Directory}Entry::getName()` #74910

jansvoboda11 · 2023-12-09T02:55:00Z

The files and directories that Clang accesses are uniqued by their inode. For each inode FileManager will create exactly one FileEntry or DirectoryEntry object, which makes answering the question "Are these two files/directories the same?" a simple pointer equality check.

However, since the same inode can be accessed through multiple different paths, asking the FileEntry or DirectoryEntry object "What is your name?" doesn't have clear semantics. In c0ff990 we started reporting the most recent name used to access the entry, which turned out to be necessary for Clang modules. However, the long-term solution has always been to explicitly track the as-requested name. This has been implemented in 4dc5573 as FileEntryRef and DirectoryEntryRef.

The DirectoryEntry::getName() interface has been deprecated since the Clang 17 release and FileEntry::getName() since Clang 18. We have replaced uses of these deprecated APIs in main with DirectoryEntryRef::getName() and FileEntryRef::getName() respectively.

This makes it possible to remove {File,Directory}Entry::getName() for good along with the FileManager code that implements them.

llvmbot · 2023-12-09T02:55:27Z

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/74910.diff

7 Files Affected:

(modified) clang/include/clang/Basic/DirectoryEntry.h (-7)
(modified) clang/include/clang/Basic/FileEntry.h (-10)
(modified) clang/lib/Basic/FileManager.cpp (-26)
(modified) clang/unittests/Basic/FileManagerTest.cpp (-6)
(modified) llvm/include/llvm/Support/VirtualFileSystem.h (-3)
(modified) llvm/lib/Support/VirtualFileSystem.cpp (-1)
(modified) llvm/unittests/Support/VirtualFileSystemTest.cpp (-18)

diff --git a/clang/include/clang/Basic/DirectoryEntry.h b/clang/include/clang/Basic/DirectoryEntry.h
index 906c2e9af23b31..35fe529ba79dff 100644
--- a/clang/include/clang/Basic/DirectoryEntry.h
+++ b/clang/include/clang/Basic/DirectoryEntry.h
@@ -41,13 +41,6 @@ class DirectoryEntry {
   DirectoryEntry &operator=(const DirectoryEntry &) = delete;
   friend class FileManager;
   friend class FileEntryTestHelper;
-
-  // FIXME: We should not be storing a directory entry name here.
-  StringRef Name; // Name of the directory.
-
-public:
-  LLVM_DEPRECATED("Use DirectoryEntryRef::getName() instead.", "")
-  StringRef getName() const { return Name; }
 };
 
 /// A reference to a \c DirectoryEntry  that includes the name of the directory
diff --git a/clang/include/clang/Basic/FileEntry.h b/clang/include/clang/Basic/FileEntry.h
index 35efa147950f06..68d4bf60930037 100644
--- a/clang/include/clang/Basic/FileEntry.h
+++ b/clang/include/clang/Basic/FileEntry.h
@@ -318,18 +318,8 @@ class FileEntry {
   /// The file content, if it is owned by the \p FileEntry.
   std::unique_ptr<llvm::MemoryBuffer> Content;
 
-  // First access name for this FileEntry.
-  //
-  // This is Optional only to allow delayed construction (FileEntryRef has no
-  // default constructor). It should always have a value in practice.
-  //
-  // TODO: remove this once everyone that needs a name uses FileEntryRef.
-  OptionalFileEntryRef LastRef;
-
 public:
   ~FileEntry();
-  LLVM_DEPRECATED("Use FileEntryRef::getName() instead.", "")
-  StringRef getName() const { return LastRef->getName(); }
 
   StringRef tryGetRealPathName() const { return RealPathName; }
   off_t getSize() const { return Size; }
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index d16626b1065213..e715e69f7bdbb0 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -107,7 +107,6 @@ void FileManager::addAncestorsAsVirtualDirs(StringRef Path) {
 
   // Add the virtual directory to the cache.
   auto *UDE = new (DirsAlloc.Allocate()) DirectoryEntry();
-  UDE->Name = NamedDirEnt.first();
   NamedDirEnt.second = *UDE;
   VirtualDirectoryEntries.push_back(UDE);
 
@@ -179,7 +178,6 @@ FileManager::getDirectoryRef(StringRef DirName, bool CacheFailure) {
     // We don't have this directory yet, add it.  We use the string
     // key from the SeenDirEntries map as the string.
     UDE = new (DirsAlloc.Allocate()) DirectoryEntry();
-    UDE->Name = InterndDirName;
   }
   NamedDirEnt.second = *UDE;
 
@@ -324,32 +322,10 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
 
   FileEntryRef ReturnedRef(*NamedFileEnt);
   if (ReusingEntry) { // Already have an entry with this inode, return it.
-
-    // FIXME: This hack ensures that `getDir()` will use the path that was
-    // used to lookup this file, even if we found a file by different path
-    // first. This is required in order to find a module's structure when its
-    // headers/module map are mapped in the VFS.
-    //
-    // See above for how this will eventually be removed. `IsVFSMapped`
-    // *cannot* be narrowed to `ExposesExternalVFSPath` as crash reproducers
-    // also depend on this logic and they have `use-external-paths: false`.
-    if (&DirInfo.getDirEntry() != UFE->Dir && Status.IsVFSMapped)
-      UFE->Dir = &DirInfo.getDirEntry();
-
-    // Always update LastRef to the last name by which a file was accessed.
-    // FIXME: Neither this nor always using the first reference is correct; we
-    // want to switch towards a design where we return a FileName object that
-    // encapsulates both the name by which the file was accessed and the
-    // corresponding FileEntry.
-    // FIXME: LastRef should be removed from FileEntry once all clients adopt
-    // FileEntryRef.
-    UFE->LastRef = ReturnedRef;
-
     return ReturnedRef;
   }
 
   // Otherwise, we don't have this file yet, add it.
-  UFE->LastRef = ReturnedRef;
   UFE->Size = Status.getSize();
   UFE->ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
   UFE->Dir = &DirInfo.getDirEntry();
@@ -461,7 +437,6 @@ FileEntryRef FileManager::getVirtualFileRef(StringRef Filename, off_t Size,
   }
 
   NamedFileEnt.second = FileEntryRef::MapValue(*UFE, *DirInfo);
-  UFE->LastRef = FileEntryRef(NamedFileEnt);
   UFE->Size    = Size;
   UFE->ModTime = ModificationTime;
   UFE->Dir     = &DirInfo->getDirEntry();
@@ -490,7 +465,6 @@ OptionalFileEntryRef FileManager::getBypassFile(FileEntryRef VF) {
   FileEntry *BFE = new (FilesAlloc.Allocate()) FileEntry();
   BypassFileEntries.push_back(BFE);
   Insertion.first->second = FileEntryRef::MapValue(*BFE, VF.getDir());
-  BFE->LastRef = FileEntryRef(*Insertion.first);
   BFE->Size = Status.getSize();
   BFE->Dir = VF.getFileEntry().Dir;
   BFE->ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
diff --git a/clang/unittests/Basic/FileManagerTest.cpp b/clang/unittests/Basic/FileManagerTest.cpp
index 43339676c4a6a2..d32036d975ce9c 100644
--- a/clang/unittests/Basic/FileManagerTest.cpp
+++ b/clang/unittests/Basic/FileManagerTest.cpp
@@ -284,9 +284,6 @@ TEST_F(FileManagerTest, getFileRefReturnsCorrectNameForDifferentStatPath) {
   ASSERT_FALSE(!F1Alias);
   ASSERT_FALSE(!F1Alias2);
   EXPECT_EQ("dir/f1.cpp", F1->getName());
-  LLVM_SUPPRESS_DEPRECATED_DECLARATIONS_PUSH
-  EXPECT_EQ("dir/f1.cpp", F1->getFileEntry().getName());
-  LLVM_SUPPRESS_DEPRECATED_DECLARATIONS_POP
   EXPECT_EQ("dir/f1.cpp", F1Alias->getName());
   EXPECT_EQ("dir/f1.cpp", F1Alias2->getName());
   EXPECT_EQ(&F1->getFileEntry(), &F1Alias->getFileEntry());
@@ -305,9 +302,6 @@ TEST_F(FileManagerTest, getFileRefReturnsCorrectNameForDifferentStatPath) {
   ASSERT_FALSE(!F2Alias);
   ASSERT_FALSE(!F2Alias2);
   EXPECT_EQ("dir/f2.cpp", F2->getName());
-  LLVM_SUPPRESS_DEPRECATED_DECLARATIONS_PUSH
-  EXPECT_EQ("dir/f2.cpp", F2->getFileEntry().getName());
-  LLVM_SUPPRESS_DEPRECATED_DECLARATIONS_POP
   EXPECT_EQ("dir/f2.cpp", F2Alias->getName());
   EXPECT_EQ("dir/f2.cpp", F2Alias2->getName());
   EXPECT_EQ(&F2->getFileEntry(), &F2Alias->getFileEntry());
diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h
index 44a56e54a0a09c..1c3d8e985592b6 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -55,9 +55,6 @@ class Status {
   llvm::sys::fs::perms Perms;
 
 public:
-  // FIXME: remove when files support multiple names
-  bool IsVFSMapped = false;
-
   /// Whether this entity has an external path different from the virtual path,
   /// and the external path is exposed by leaking it through the abstraction.
   /// For example, a RedirectingFileSystem will set this for paths where
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp
index 367e794d38f63a..9fed081f85a50a 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -2334,7 +2334,6 @@ static Status getRedirectedFileStatus(const Twine &OriginalPath,
     S = Status::copyWithNewName(S, OriginalPath);
   else
     S.ExposesExternalVFSPath = true;
-  S.IsVFSMapped = true;
   return S;
 }
 
diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp
index bd4048f025c0de..87b794b05b48f7 100644
--- a/llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1563,13 +1563,11 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   ErrorOr<vfs::Status> S = O->status("//root/file1");
   ASSERT_FALSE(S.getError());
   EXPECT_EQ("//root/foo/bar/a", S->getName());
-  EXPECT_TRUE(S->IsVFSMapped);
   EXPECT_TRUE(S->ExposesExternalVFSPath);
 
   ErrorOr<vfs::Status> SLower = O->status("//root/foo/bar/a");
   EXPECT_EQ("//root/foo/bar/a", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
   EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file after opening
@@ -1578,7 +1576,6 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
   EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   // directory
@@ -1591,21 +1588,18 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   S = O->status("//root/mappeddir");
   ASSERT_FALSE(S.getError());
   EXPECT_TRUE(S->isDirectory());
-  EXPECT_TRUE(S->IsVFSMapped);
   EXPECT_TRUE(S->ExposesExternalVFSPath);
   EXPECT_TRUE(S->equivalent(*O->status("//root/foo/bar")));
 
   SLower = O->status("//root/foo/bar");
   EXPECT_EQ("//root/foo/bar", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
   EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file in remapped directory
   S = O->status("//root/mappeddir/a");
   ASSERT_FALSE(S.getError());
   EXPECT_FALSE(S->isDirectory());
-  EXPECT_TRUE(S->IsVFSMapped);
   EXPECT_TRUE(S->ExposesExternalVFSPath);
   EXPECT_EQ("//root/foo/bar/a", S->getName());
 
@@ -1613,7 +1607,6 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   S = O->status("//root/mappeddir2/a");
   ASSERT_FALSE(S.getError());
   EXPECT_FALSE(S->isDirectory());
-  EXPECT_TRUE(S->IsVFSMapped);
   EXPECT_FALSE(S->ExposesExternalVFSPath);
   EXPECT_EQ("//root/mappeddir2/a", S->getName());
 
@@ -1623,7 +1616,6 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
   EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   // file contents in remapped directory, with use-external-name=false
@@ -1632,7 +1624,6 @@ TEST_F(VFSFromYAMLTest, MappedFiles) {
   OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/mappeddir2/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
   EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   // broken mapping
@@ -1665,13 +1656,11 @@ TEST_F(VFSFromYAMLTest, MappedRoot) {
   ErrorOr<vfs::Status> S = O->status("//mappedroot/a");
   ASSERT_FALSE(S.getError());
   EXPECT_EQ("//root/foo/bar/a", S->getName());
-  EXPECT_TRUE(S->IsVFSMapped);
   EXPECT_TRUE(S->ExposesExternalVFSPath);
 
   ErrorOr<vfs::Status> SLower = O->status("//root/foo/bar/a");
   EXPECT_EQ("//root/foo/bar/a", SLower->getName());
   EXPECT_TRUE(S->equivalent(*SLower));
-  EXPECT_FALSE(SLower->IsVFSMapped);
   EXPECT_FALSE(SLower->ExposesExternalVFSPath);
 
   // file after opening
@@ -1680,7 +1669,6 @@ TEST_F(VFSFromYAMLTest, MappedRoot) {
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("//root/foo/bar/a", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
   EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
@@ -1829,13 +1817,11 @@ TEST_F(VFSFromYAMLTest, ReturnsRequestedPathVFSMiss) {
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("a", OpenedS->getName());
-  EXPECT_FALSE(OpenedS->IsVFSMapped);
   EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   auto DirectS = RemappedFS->status("a");
   ASSERT_FALSE(DirectS.getError());
   EXPECT_EQ("a", DirectS->getName());
-  EXPECT_FALSE(DirectS->IsVFSMapped);
   EXPECT_FALSE(DirectS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
@@ -1871,13 +1857,11 @@ TEST_F(VFSFromYAMLTest, ReturnsExternalPathVFSHit) {
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("realname", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
   EXPECT_TRUE(OpenedS->ExposesExternalVFSPath);
 
   auto DirectS = FS->status("vfsname");
   ASSERT_FALSE(DirectS.getError());
   EXPECT_EQ("realname", DirectS->getName());
-  EXPECT_TRUE(DirectS->IsVFSMapped);
   EXPECT_TRUE(DirectS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);
@@ -1976,13 +1960,11 @@ TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   auto OpenedS = (*OpenedF)->status();
   ASSERT_FALSE(OpenedS.getError());
   EXPECT_EQ("vfsname", OpenedS->getName());
-  EXPECT_TRUE(OpenedS->IsVFSMapped);
   EXPECT_FALSE(OpenedS->ExposesExternalVFSPath);
 
   auto DirectS = FS->status("vfsname");
   ASSERT_FALSE(DirectS.getError());
   EXPECT_EQ("vfsname", DirectS->getName());
-  EXPECT_TRUE(DirectS->IsVFSMapped);
   EXPECT_FALSE(DirectS->ExposesExternalVFSPath);
 
   EXPECT_EQ(0, NumDiagnostics);

jansvoboda11 · 2023-12-11T22:51:16Z

Note: I'd like to merge this PR after we branch off for Clang 18 in January or February 2024. I had the patch lying around, so I figured I might as well create the PR.

dexonsmith

Amazing! LGTM, once the branch is clear.

jansvoboda11 · 2024-01-24T16:41:01Z

The release/18.x branch has been created. Merging...

@zsrkmyn

Based on a patch by @zsrkmyn

@zsrkmyn

Based on a patch by @zsrkmyn

It does not compile with newer version of LLVM because `FileEntry::getName()` was removed by this llvm/llvm-project#74910. It has been deprecated for more than two years: llvm/llvm-project@6a79e2f

It does not compile with newer version of LLVM because `FileEntry::getName()` was removed by this llvm/llvm-project#74910. The alternative has been available for more than two years: llvm/llvm-project@6a79e2f

GKxxUCAS · 2024-10-30T19:15:36Z

What is the preferred way of knowing the name of a file given its FileEntry? In particular I want to check if a certain header foo.h is included. Before the removal of FileEntry::getName() I could write

for (const auto *FE : CI.getPreprocessor().getIncludedFiles())
  if (FE->getName() == "foo.h")
    // do something

How can I achieve that now?

dexonsmith · 2024-10-30T19:42:56Z

Perhaps `IncludedFilesSet` / `getIncludedFiles()` should be updated to traffic in `FileEntryRef` instead of `FileEntry`.

…

On Oct 30, 2024, at 15:15, GKxxUCAS ***@***.***> wrote: What is the preferred way of knowing the name of a file given its FileEntry? In particular I want to check if a certain header foo.h is included. Before the removal of FileEntry::getName() I could write for (const auto *FE : CI.getPreprocessor().getIncludedFiles()) if (FE->getName() == "foo.h") // do something How can I achieve that now? — Reply to this email directly, view it on GitHub <#74910 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKF2XHJR5EDXBHFW4HVGZLZ6EV67AVCNFSM6AAAAABQ42GWAGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBYGE2DCNZTGI>. You are receiving this because your review was requested.

jansvoboda11 · 2024-10-30T20:38:34Z

How about something like this?

auto FE = CI.getFileManager().getOptionalFileEntryRef("foo.h");
bool IsFooIncluded = FE && CI.getPreprocessor().getIncludedFiles().contains(*FE);

GKxxUCAS · 2024-11-08T18:45:49Z

@jansvoboda11 did not work for me. I ended up writing a PPCallbacks and overriding InclusionDirective where I could get plenty of information of an #include directive.

[clang] NFC: Remove {File,Directory}Entry::getName()

f1a4ff8

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" llvm:support labels Dec 9, 2023

jansvoboda11 requested review from sam-mccall, dexonsmith, bnbarham and benlangmuir December 11, 2023 22:50

dexonsmith approved these changes Dec 11, 2023

View reviewed changes

benlangmuir approved these changes Dec 11, 2023

View reviewed changes

jansvoboda11 merged commit 6c1dbd5 into main Jan 24, 2024

jansvoboda11 deleted the users/jansvoboda11/remove-get-name branch January 24, 2024 16:41

MaskRay added a commit to MaskRay/ccls that referenced this pull request Feb 2, 2024

Adapt llvm/llvm-project#74910

af15dc9

Based on a patch by @zsrkmyn

MaskRay added a commit to MaskRay/ccls that referenced this pull request Feb 2, 2024

Adapt llvm/llvm-project#74910: FileEntry::getName

f8d2778

Based on a patch by @zsrkmyn

kostadinsh mentioned this pull request Nov 27, 2024

Build failure with Clang 19 SUSE/clang-extract#134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] NFC: Remove `{File,Directory}Entry::getName()` #74910

[clang] NFC: Remove `{File,Directory}Entry::getName()` #74910

Uh oh!

jansvoboda11 commented Dec 9, 2023 •

edited

Loading

Uh oh!

llvmbot commented Dec 9, 2023 •

edited

Loading

Uh oh!

jansvoboda11 commented Dec 11, 2023

Uh oh!

dexonsmith left a comment

Uh oh!

jansvoboda11 commented Jan 24, 2024

Uh oh!

GKxxUCAS commented Oct 30, 2024

Uh oh!

dexonsmith commented Oct 30, 2024 via email

Uh oh!

jansvoboda11 commented Oct 30, 2024

Uh oh!

GKxxUCAS commented Nov 8, 2024

Uh oh!

Uh oh!

[clang] NFC: Remove {File,Directory}Entry::getName() #74910

[clang] NFC: Remove {File,Directory}Entry::getName() #74910

Uh oh!

Conversation

jansvoboda11 commented Dec 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jansvoboda11 commented Dec 11, 2023

Uh oh!

dexonsmith left a comment

Choose a reason for hiding this comment

Uh oh!

jansvoboda11 commented Jan 24, 2024

Uh oh!

GKxxUCAS commented Oct 30, 2024

Uh oh!

dexonsmith commented Oct 30, 2024 via email

Uh oh!

jansvoboda11 commented Oct 30, 2024

Uh oh!

GKxxUCAS commented Nov 8, 2024

Uh oh!

Uh oh!

[clang] NFC: Remove `{File,Directory}Entry::getName()` #74910

[clang] NFC: Remove `{File,Directory}Entry::getName()` #74910

jansvoboda11 commented Dec 9, 2023 •

edited

Loading

llvmbot commented Dec 9, 2023 •

edited

Loading