[PATCH] D65481: NFCI: Simplify SourceManager::translateFile by removing code path that should never be taken
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL369585: NFCI: Simplify SourceManager::translateFile by
removing code path that should… (authored by arphaman, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D65481?vs=212454=216484#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65481/new/
https://reviews.llvm.org/D65481
Files:
cfe/trunk/lib/Basic/SourceManager.cpp
Index: cfe/trunk/lib/Basic/SourceManager.cpp
===
--- cfe/trunk/lib/Basic/SourceManager.cpp
+++ cfe/trunk/lib/Basic/SourceManager.cpp
@@ -1558,22 +1558,6 @@
// Other miscellaneous methods.
//===--===//
-/// Retrieve the inode for the given file entry, if possible.
-///
-/// This routine involves a system call, and therefore should only be used
-/// in non-performance-critical code.
-static Optional
-getActualFileUID(const FileEntry *File) {
- if (!File)
-return None;
-
- llvm::sys::fs::UniqueID ID;
- if (llvm::sys::fs::getUniqueID(File->getName(), ID))
-return None;
-
- return ID;
-}
-
/// Get the source location for the given file:line:col triplet.
///
/// If the source file is included multiple times, the source location will
@@ -1595,13 +1579,8 @@
FileID SourceManager::translateFile(const FileEntry *SourceFile) const {
assert(SourceFile && "Null source file!");
- // Find the first file ID that corresponds to the given file.
- FileID FirstFID;
-
// First, check the main file ID, since it is common to look for a
// location in the main file.
- Optional SourceFileUID;
- Optional SourceFileName;
if (MainFileID.isValid()) {
bool Invalid = false;
const SLocEntry = getSLocEntry(MainFileID, );
@@ -1609,100 +1588,35 @@
return FileID();
if (MainSLoc.isFile()) {
- const ContentCache *MainContentCache
-= MainSLoc.getFile().getContentCache();
- if (!MainContentCache || !MainContentCache->OrigEntry) {
-// Can't do anything
- } else if (MainContentCache->OrigEntry == SourceFile) {
-FirstFID = MainFileID;
- } else {
-// Fall back: check whether we have the same base name and inode
-// as the main file.
-const FileEntry *MainFile = MainContentCache->OrigEntry;
-SourceFileName = llvm::sys::path::filename(SourceFile->getName());
-if (*SourceFileName == llvm::sys::path::filename(MainFile->getName())) {
- SourceFileUID = getActualFileUID(SourceFile);
- if (SourceFileUID) {
-if (Optional MainFileUID =
-getActualFileUID(MainFile)) {
- if (*SourceFileUID == *MainFileUID) {
-FirstFID = MainFileID;
-SourceFile = MainFile;
- }
-}
- }
-}
- }
+ const ContentCache *MainContentCache =
+ MainSLoc.getFile().getContentCache();
+ if (MainContentCache && MainContentCache->OrigEntry == SourceFile)
+return MainFileID;
}
}
- if (FirstFID.isInvalid()) {
-// The location we're looking for isn't in the main file; look
-// through all of the local source locations.
-for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
- bool Invalid = false;
- const SLocEntry = getLocalSLocEntry(I, );
- if (Invalid)
-return FileID();
+ // The location we're looking for isn't in the main file; look
+ // through all of the local source locations.
+ for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
+bool Invalid = false;
+const SLocEntry = getLocalSLocEntry(I, );
+if (Invalid)
+ return FileID();
- if (SLoc.isFile() &&
- SLoc.getFile().getContentCache() &&
- SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
-FirstFID = FileID::get(I);
-break;
- }
-}
-// If that still didn't help, try the modules.
-if (FirstFID.isInvalid()) {
- for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
-const SLocEntry = getLoadedSLocEntry(I);
-if (SLoc.isFile() &&
-SLoc.getFile().getContentCache() &&
-SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
- FirstFID = FileID::get(-int(I) - 2);
- break;
-}
- }
-}
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile)
+ return FileID::get(I);
}
- // If we haven't found what we want yet, try again, but this time stat()
- // each of the files in case the files have changed since we originally
- // parsed the file.
- if (FirstFID.isInvalid() &&
[PATCH] D65481: NFCI: Simplify SourceManager::translateFile by removing code path that should never be taken
arphaman added a comment. Ping. I will commit it this week if there are no objections. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65481/new/ https://reviews.llvm.org/D65481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65481: NFCI: Simplify SourceManager::translateFile by removing code path that should never be taken
arphaman created this revision.
arphaman added reviewers: rsmith, bruno, Bigcheese.
Herald added subscribers: dexonsmith, jkorous.
Herald added a project: clang.
I noticed that `SourceManager::translateFile` has code that doesn't really make
sense. In particular, if it fails to find a `FileID` by comparing `FileEntry *`
values, it tries to look through files that have the same filename, to see if
they have a matching inode to try to find the right `FileID`. However, the
inode comparison seem redundant, as Clang's `FileManager` already deduplicates
`FileEntry *` values by inode. Thus the comparisons between inodes should never
actually succeed, and the comparison between `FileEntry *` values should be
sufficient here.
This observation is supported by the code coverage report that shows that we
never actually reach the case where the INODE comparison succeeds:
http://lab.llvm.org:8080/coverage/coverage-reports/clang/coverage/Users/buildslave/jenkins/workspace/clang-stage2-coverage-R/llvm/tools/clang/lib/Basic/SourceManager.cpp.html#L1595
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D65481
Files:
clang/lib/Basic/SourceManager.cpp
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -1558,22 +1558,6 @@
// Other miscellaneous methods.
//===--===//
-/// Retrieve the inode for the given file entry, if possible.
-///
-/// This routine involves a system call, and therefore should only be used
-/// in non-performance-critical code.
-static Optional
-getActualFileUID(const FileEntry *File) {
- if (!File)
-return None;
-
- llvm::sys::fs::UniqueID ID;
- if (llvm::sys::fs::getUniqueID(File->getName(), ID))
-return None;
-
- return ID;
-}
-
/// Get the source location for the given file:line:col triplet.
///
/// If the source file is included multiple times, the source location will
@@ -1595,13 +1579,8 @@
FileID SourceManager::translateFile(const FileEntry *SourceFile) const {
assert(SourceFile && "Null source file!");
- // Find the first file ID that corresponds to the given file.
- FileID FirstFID;
-
// First, check the main file ID, since it is common to look for a
// location in the main file.
- Optional SourceFileUID;
- Optional SourceFileName;
if (MainFileID.isValid()) {
bool Invalid = false;
const SLocEntry = getSLocEntry(MainFileID, );
@@ -1609,100 +1588,35 @@
return FileID();
if (MainSLoc.isFile()) {
- const ContentCache *MainContentCache
-= MainSLoc.getFile().getContentCache();
- if (!MainContentCache || !MainContentCache->OrigEntry) {
-// Can't do anything
- } else if (MainContentCache->OrigEntry == SourceFile) {
-FirstFID = MainFileID;
- } else {
-// Fall back: check whether we have the same base name and inode
-// as the main file.
-const FileEntry *MainFile = MainContentCache->OrigEntry;
-SourceFileName = llvm::sys::path::filename(SourceFile->getName());
-if (*SourceFileName == llvm::sys::path::filename(MainFile->getName())) {
- SourceFileUID = getActualFileUID(SourceFile);
- if (SourceFileUID) {
-if (Optional MainFileUID =
-getActualFileUID(MainFile)) {
- if (*SourceFileUID == *MainFileUID) {
-FirstFID = MainFileID;
-SourceFile = MainFile;
- }
-}
- }
-}
- }
+ const ContentCache *MainContentCache =
+ MainSLoc.getFile().getContentCache();
+ if (MainContentCache && MainContentCache->OrigEntry == SourceFile)
+return MainFileID;
}
}
- if (FirstFID.isInvalid()) {
-// The location we're looking for isn't in the main file; look
-// through all of the local source locations.
-for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
- bool Invalid = false;
- const SLocEntry = getLocalSLocEntry(I, );
- if (Invalid)
-return FileID();
+ // The location we're looking for isn't in the main file; look
+ // through all of the local source locations.
+ for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
+bool Invalid = false;
+const SLocEntry = getLocalSLocEntry(I, );
+if (Invalid)
+ return FileID();
- if (SLoc.isFile() &&
- SLoc.getFile().getContentCache() &&
- SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
-FirstFID = FileID::get(I);
-break;
- }
-}
-// If that still didn't help, try the modules.
-if (FirstFID.isInvalid()) {
- for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
-const SLocEntry = getLoadedSLocEntry(I);
-if (SLoc.isFile() &&
-SLoc.getFile().getContentCache()
