[PATCH] D58924: Replace clang::FileData with llvm::vfs::Status
This revision was automatically updated to reflect the committed changes.
Closed by commit rL355368: Replace clang::FileData with llvm::vfs::Status
(authored by harlanhaskins, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D58924?vs=189202=189258#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58924/new/
https://reviews.llvm.org/D58924
Files:
cfe/trunk/include/clang/Basic/FileManager.h
cfe/trunk/include/clang/Basic/FileSystemStatCache.h
cfe/trunk/lib/Basic/FileManager.cpp
cfe/trunk/lib/Basic/FileSystemStatCache.cpp
cfe/trunk/lib/Frontend/TextDiagnostic.cpp
cfe/trunk/unittests/Basic/FileManagerTest.cpp
Index: cfe/trunk/lib/Basic/FileManager.cpp
===
--- cfe/trunk/lib/Basic/FileManager.cpp
+++ cfe/trunk/lib/Basic/FileManager.cpp
@@ -144,8 +144,8 @@
StringRef InterndDirName = NamedDirEnt.first();
// Check to see if the directory exists.
- FileData Data;
- if (getStatValue(InterndDirName, Data, false, nullptr /*directory lookup*/)) {
+ llvm::vfs::Status Status;
+ if (getStatValue(InterndDirName, Status, false, nullptr /*directory lookup*/)) {
// There's no real directory at the given path.
if (!CacheFailure)
SeenDirEntries.erase(DirName);
@@ -156,7 +156,7 @@
// same inode (this occurs on Unix-like systems when one dir is
// symlinked to another, for example) or the same path (on
// Windows).
- DirectoryEntry = UniqueRealDirs[Data.UniqueID];
+ DirectoryEntry = UniqueRealDirs[Status.getUniqueID()];
NamedDirEnt.second =
if (UDE.getName().empty()) {
@@ -205,8 +205,8 @@
// Check to see if the file exists.
std::unique_ptr F;
- FileData Data;
- if (getStatValue(InterndFileName, Data, true, openFile ? : nullptr)) {
+ llvm::vfs::Status Status;
+ if (getStatValue(InterndFileName, Status, true, openFile ? : nullptr)) {
// There's no real file at the given path.
if (!CacheFailure)
SeenFileEntries.erase(Filename);
@@ -218,14 +218,15 @@
// It exists. See if we have already opened a file with the same inode.
// This occurs when one dir is symlinked to another, for example.
- FileEntry = UniqueRealFiles[Data.UniqueID];
+ FileEntry = UniqueRealFiles[Status.getUniqueID()];
NamedFileEnt.second =
// If the name returned by getStatValue is different than Filename, re-intern
// the name.
- if (Data.Name != Filename) {
-auto = *SeenFileEntries.insert({Data.Name, }).first;
+ if (Status.getName() != Filename) {
+auto =
+ *SeenFileEntries.insert({Status.getName(), }).first;
assert(NamedFileEnt.second == &&
"filename from getStatValue() refers to wrong file");
InterndFileName = NamedFileEnt.first().data();
@@ -239,7 +240,7 @@
// module's structure when its headers/module map are mapped in the VFS.
// We should remove this as soon as we can properly support a file having
// multiple names.
-if (DirInfo != UFE.Dir && Data.IsVFSMapped)
+if (DirInfo != UFE.Dir && Status.IsVFSMapped)
UFE.Dir = DirInfo;
// Always update the name to use the last name by which a file was accessed.
@@ -254,13 +255,12 @@
// Otherwise, we don't have this file yet, add it.
UFE.Name= InterndFileName;
- UFE.Size = Data.Size;
- UFE.ModTime = Data.ModTime;
+ UFE.Size= Status.getSize();
+ UFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
UFE.Dir = DirInfo;
UFE.UID = NextFileUID++;
- UFE.UniqueID = Data.UniqueID;
- UFE.IsNamedPipe = Data.IsNamedPipe;
- UFE.InPCH = Data.InPCH;
+ UFE.UniqueID = Status.getUniqueID();
+ UFE.IsNamedPipe = Status.getType() == llvm::sys::fs::file_type::fifo_file;
UFE.File = std::move(F);
UFE.IsValid = true;
@@ -298,12 +298,15 @@
"The directory of a virtual file should already be in the cache.");
// Check to see if the file exists. If so, drop the virtual file
- FileData Data;
+ llvm::vfs::Status Status;
const char *InterndFileName = NamedFileEnt.first().data();
- if (getStatValue(InterndFileName, Data, true, nullptr) == 0) {
-Data.Size = Size;
-Data.ModTime = ModificationTime;
-UFE = [Data.UniqueID];
+ if (getStatValue(InterndFileName, Status, true, nullptr) == 0) {
+UFE = [Status.getUniqueID()];
+Status = llvm::vfs::Status(
+ Status.getName(), Status.getUniqueID(),
+ llvm::sys::toTimePoint(ModificationTime),
+ Status.getUser(), Status.getGroup(), Size,
+ Status.getType(), Status.getPermissions());
NamedFileEnt.second = UFE;
@@ -317,10 +320,9 @@
if (UFE->isValid())
return UFE;
-UFE->UniqueID = Data.UniqueID;
-UFE->IsNamedPipe = Data.IsNamedPipe;
-UFE->InPCH = Data.InPCH;
-fillRealPathName(UFE, Data.Name);
+UFE->UniqueID = Status.getUniqueID();
+UFE->IsNamedPipe = Status.getType() ==
[PATCH] D58924: Replace clang::FileData with llvm::vfs::Status
benlangmuir added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:305 +UFE = [Status.getUniqueID()]; +Status = llvm::vfs::Status( + Status.getName(), Status.getUniqueID(), harlanhaskins wrote: > benlangmuir wrote: > > Why copy Status back into Status instead of mutating the relevant fields? > The fields don't have setters exposed, and I couldn't decide if adding the > setters vs. re-constructing a Status was better here. Would it be worth > adding setters to the properties in `Status`? Nah, just go with the code you already wrote. Thanks for the explanation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58924/new/ https://reviews.llvm.org/D58924 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58924: Replace clang::FileData with llvm::vfs::Status
harlanhaskins marked an inline comment as done. harlanhaskins added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:305 +UFE = [Status.getUniqueID()]; +Status = llvm::vfs::Status( + Status.getName(), Status.getUniqueID(), benlangmuir wrote: > Why copy Status back into Status instead of mutating the relevant fields? The fields don't have setters exposed, and I couldn't decide if adding the setters vs. re-constructing a Status was better here. Would it be worth adding setters to the properties in `Status`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58924/new/ https://reviews.llvm.org/D58924 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58924: Replace clang::FileData with llvm::vfs::Status
benlangmuir accepted this revision. benlangmuir added a comment. This revision is now accepted and ready to land. LGTM, although I made a small suggestion for clarity. FYI `InPCH` was used by PTH, which was removed a couple of months ago. Comment at: clang/lib/Basic/FileManager.cpp:305 +UFE = [Status.getUniqueID()]; +Status = llvm::vfs::Status( + Status.getName(), Status.getUniqueID(), Why copy Status back into Status instead of mutating the relevant fields? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58924/new/ https://reviews.llvm.org/D58924 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58924: Replace clang::FileData with llvm::vfs::Status
harlanhaskins created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
harlanhaskins edited the summary of this revision.
harlanhaskins added a reviewer: benlangmuir.
Herald added a subscriber: ormris.
`FileData` was only ever used as a container for the values in
`llvm::vfs::Status`, so they might as well be consolidated.
The `InPCH` member was also always set to false, and unused.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D58924
Files:
clang/include/clang/Basic/FileManager.h
clang/include/clang/Basic/FileSystemStatCache.h
clang/lib/Basic/FileManager.cpp
clang/lib/Basic/FileSystemStatCache.cpp
clang/lib/Frontend/TextDiagnostic.cpp
clang/unittests/Basic/FileManagerTest.cpp
Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -26,7 +26,7 @@
private:
// Maps a file/directory path to its desired stat result. Anything
// not in this map is considered to not exist in the file system.
- llvm::StringMap StatCalls;
+ llvm::StringMap StatCalls;
void InjectFileOrDirectory(const char *Path, ino_t INode, bool IsFile) {
#ifndef _WIN32
@@ -35,15 +35,14 @@
Path = NormalizedPath.c_str();
#endif
-FileData Data;
-Data.Name = Path;
-Data.Size = 0;
-Data.ModTime = 0;
-Data.UniqueID = llvm::sys::fs::UniqueID(1, INode);
-Data.IsDirectory = !IsFile;
-Data.IsNamedPipe = false;
-Data.InPCH = false;
-StatCalls[Path] = Data;
+auto fileType = IsFile ?
+ llvm::sys::fs::file_type::regular_file :
+ llvm::sys::fs::file_type::directory_file;
+llvm::vfs::Status Status(Path, llvm::sys::fs::UniqueID(1, INode),
+ /*MTime*/{}, /*User*/0, /*Group*/0,
+ /*Size*/0, fileType,
+ llvm::sys::fs::perms::all_all);
+StatCalls[Path] = Status;
}
public:
@@ -58,7 +57,7 @@
}
// Implement FileSystemStatCache::getStat().
- LookupResult getStat(StringRef Path, FileData , bool isFile,
+ LookupResult getStat(StringRef Path, llvm::vfs::Status , bool isFile,
std::unique_ptr *F,
llvm::vfs::FileSystem ) override {
#ifndef _WIN32
@@ -68,7 +67,7 @@
#endif
if (StatCalls.count(Path) != 0) {
- Data = StatCalls[Path];
+ Status = StatCalls[Path];
return CacheExists;
}
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -792,8 +792,6 @@
const FileEntry *FE = Loc.getFileEntry();
if (FE && FE->isValid()) {
emitFilename(FE->getName(), Loc.getManager());
-if (FE->isInPCH())
- OS << " (in PCH)";
OS << ": ";
}
}
Index: clang/lib/Basic/FileSystemStatCache.cpp
===
--- clang/lib/Basic/FileSystemStatCache.cpp
+++ clang/lib/Basic/FileSystemStatCache.cpp
@@ -21,18 +21,6 @@
void FileSystemStatCache::anchor() {}
-static void copyStatusToFileData(const llvm::vfs::Status ,
- FileData ) {
- Data.Name = Status.getName();
- Data.Size = Status.getSize();
- Data.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
- Data.UniqueID = Status.getUniqueID();
- Data.IsDirectory = Status.isDirectory();
- Data.IsNamedPipe = Status.getType() == llvm::sys::fs::file_type::fifo_file;
- Data.InPCH = false;
- Data.IsVFSMapped = Status.IsVFSMapped;
-}
-
/// FileSystemStatCache::get - Get the 'stat' information for the specified
/// path, using the cache to accelerate it if possible. This returns true if
/// the path does not exist or false if it exists.
@@ -42,7 +30,8 @@
/// success for directories (not files). On a successful file lookup, the
/// implementation can optionally fill in FileDescriptor with a valid
/// descriptor and the client guarantees that it will close it.
-bool FileSystemStatCache::get(StringRef Path, FileData , bool isFile,
+bool FileSystemStatCache::get(StringRef Path, llvm::vfs::Status ,
+ bool isFile,
std::unique_ptr *F,
FileSystemStatCache *Cache,
llvm::vfs::FileSystem ) {
@@ -51,16 +40,16 @@
// If we have a cache, use it to resolve the stat query.
if (Cache)
-R = Cache->getStat(Path, Data, isFile, F, FS);
+R = Cache->getStat(Path, Status, isFile, F, FS);
else if (isForDir || !F) {
// If this is a directory or a file descriptor is not needed and we have
// no cache, just go to the file system.
-llvm::ErrorOr Status = FS.status(Path);
-if (!Status) {
+llvm::ErrorOr StatusOrErr = FS.status(Path);
+if
