[clang] NFCI: Use FileEntryRef for FileID creation
#67838
Merged
+20
−40
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-tidy ChangesThis patch removes the Full diff: https://github.com/llvm/llvm-project/pull/67838.diff 11 Files Affected:
diff --git a/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp b/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
index 5f30cbf8fb4773e..cdff4f452205ed4 100644
--- a/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
+++ b/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
@@ -152,7 +152,7 @@ int main(int argc, const char **argv) {
for (auto I = ChangedFiles.begin(), E = ChangedFiles.end(); I != E; ++I) {
OS << " {\n";
OS << " \"FilePath\": \"" << *I << "\",\n";
- const auto Entry = FileMgr.getFile(*I);
+ auto Entry = FileMgr.getOptionalFileRef(*I);
auto ID = Sources.getOrCreateFileID(*Entry, SrcMgr::C_User);
std::string Content;
llvm::raw_string_ostream ContentStream(Content);
@@ -170,7 +170,7 @@ int main(int argc, const char **argv) {
}
for (const auto &File : ChangedFiles) {
- const auto Entry = FileMgr.getFile(File);
+ auto Entry = FileMgr.getOptionalFileRef(File);
auto ID = Sources.getOrCreateFileID(*Entry, SrcMgr::C_User);
outs() << "============== " << File << " ==============\n";
diff --git a/clang-tools-extra/clang-move/Move.cpp b/clang-tools-extra/clang-move/Move.cpp
index 94ba73ce9ad5a15..404acf55e3aa53f 100644
--- a/clang-tools-extra/clang-move/Move.cpp
+++ b/clang-tools-extra/clang-move/Move.cpp
@@ -843,7 +843,7 @@ void ClangMoveTool::moveDeclsToNewFiles() {
// Move all contents from OldFile to NewFile.
void ClangMoveTool::moveAll(SourceManager &SM, StringRef OldFile,
StringRef NewFile) {
- auto FE = SM.getFileManager().getFile(makeAbsolutePath(OldFile));
+ auto FE = SM.getFileManager().getOptionalFileRef(makeAbsolutePath(OldFile));
if (!FE) {
llvm::errs() << "Failed to get file: " << OldFile << "\n";
return;
diff --git a/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp b/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
index 375306765a52b6c..9d5536d27995fe7 100644
--- a/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
+++ b/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
@@ -84,7 +84,7 @@ int main(int argc, const char **argv) {
Tool.applyAllReplacements(Rewrite);
for (const auto &File : Files) {
- auto Entry = FileMgr.getFile(File);
+ auto Entry = FileMgr.getOptionalFileRef(File);
const auto ID = Sources.getOrCreateFileID(*Entry, SrcMgr::C_User);
Rewrite.getEditBuffer(ID).write(outs());
}
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 695bfd6e2edf7ef..4b1a67b6dd98a94 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -243,7 +243,7 @@ class ErrorReporter {
if (FilePath.empty())
return {};
- auto File = SourceMgr.getFileManager().getFile(FilePath);
+ auto File = SourceMgr.getFileManager().getOptionalFileRef(FilePath);
if (!File)
return {};
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index c1e4437b88949b9..8d7c1b088f037a9 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -194,8 +194,8 @@ DiagnosticBuilder ClangTidyContext::diag(
DiagnosticBuilder ClangTidyContext::diag(const tooling::Diagnostic &Error) {
SourceManager &SM = DiagEngine->getSourceManager();
- llvm::ErrorOr<const FileEntry *> File =
- SM.getFileManager().getFile(Error.Message.FilePath);
+ OptionalFileEntryRef File =
+ SM.getFileManager().getOptionalFileRef(Error.Message.FilePath);
FileID ID = SM.getOrCreateFileID(*File, SrcMgr::C_User);
SourceLocation FileStartLoc = SM.getLocForStartOfFile(ID);
SourceLocation Loc = FileStartLoc.getLocWithOffset(
diff --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h
index 4abb9a19622871b..62d0435755b083e 100644
--- a/clang/include/clang/Basic/SourceManager.h
+++ b/clang/include/clang/Basic/SourceManager.h
@@ -876,13 +876,6 @@ class SourceManager : public RefCountedBase<SourceManager> {
/// Create a new FileID that represents the specified file
/// being \#included from the specified IncludePosition.
- ///
- /// This translates NULL into standard input.
- FileID createFileID(const FileEntry *SourceFile, SourceLocation IncludePos,
- SrcMgr::CharacteristicKind FileCharacter,
- int LoadedID = 0,
- SourceLocation::UIntTy LoadedOffset = 0);
-
FileID createFileID(FileEntryRef SourceFile, SourceLocation IncludePos,
SrcMgr::CharacteristicKind FileCharacter,
int LoadedID = 0,
@@ -908,7 +901,7 @@ class SourceManager : public RefCountedBase<SourceManager> {
/// Get the FileID for \p SourceFile if it exists. Otherwise, create a
/// new FileID for the \p SourceFile.
- FileID getOrCreateFileID(const FileEntry *SourceFile,
+ FileID getOrCreateFileID(FileEntryRef SourceFile,
SrcMgr::CharacteristicKind FileCharacter);
/// Creates an expansion SLocEntry for the substitution of an argument into a
diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
index f1a81de329319a0..07197e19098ef39 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -527,17 +527,6 @@ FileID SourceManager::getNextFileID(FileID FID) const {
/// Create a new FileID that represents the specified file
/// being \#included from the specified IncludePosition.
-///
-/// This translates NULL into standard input.
-FileID SourceManager::createFileID(const FileEntry *SourceFile,
- SourceLocation IncludePos,
- SrcMgr::CharacteristicKind FileCharacter,
- int LoadedID,
- SourceLocation::UIntTy LoadedOffset) {
- return createFileID(SourceFile->getLastRef(), IncludePos, FileCharacter,
- LoadedID, LoadedOffset);
-}
-
FileID SourceManager::createFileID(FileEntryRef SourceFile,
SourceLocation IncludePos,
SrcMgr::CharacteristicKind FileCharacter,
@@ -585,7 +574,7 @@ FileID SourceManager::createFileID(const llvm::MemoryBufferRef &Buffer,
/// Get the FileID for \p SourceFile if it exists. Otherwise, create a
/// new FileID for the \p SourceFile.
FileID
-SourceManager::getOrCreateFileID(const FileEntry *SourceFile,
+SourceManager::getOrCreateFileID(FileEntryRef SourceFile,
SrcMgr::CharacteristicKind FileCharacter) {
FileID ID = translateFile(SourceFile);
return ID.isValid() ? ID : createFileID(SourceFile, SourceLocation(),
@@ -2375,7 +2364,7 @@ SourceManagerForFile::SourceManagerForFile(StringRef FileName,
IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs),
new DiagnosticOptions);
SourceMgr = std::make_unique<SourceManager>(*Diagnostics, *FileMgr);
- FileID ID = SourceMgr->createFileID(*FileMgr->getFile(FileName),
+ FileID ID = SourceMgr->createFileID(*FileMgr->getOptionalFileRef(FileName),
SourceLocation(), clang::SrcMgr::C_User);
assert(ID.isValid());
SourceMgr->setMainFileID(ID);
diff --git a/clang/lib/Tooling/Core/Replacement.cpp b/clang/lib/Tooling/Core/Replacement.cpp
index 2c472df086d5ec5..269f17a6db4cfc1 100644
--- a/clang/lib/Tooling/Core/Replacement.cpp
+++ b/clang/lib/Tooling/Core/Replacement.cpp
@@ -67,7 +67,7 @@ bool Replacement::isApplicable() const {
bool Replacement::apply(Rewriter &Rewrite) const {
SourceManager &SM = Rewrite.getSourceMgr();
- auto Entry = SM.getFileManager().getFile(FilePath);
+ auto Entry = SM.getFileManager().getOptionalFileRef(FilePath);
if (!Entry)
return false;
diff --git a/clang/lib/Tooling/Refactoring.cpp b/clang/lib/Tooling/Refactoring.cpp
index d45cd8c57f10e92..e3ae14f58f9f9eb 100644
--- a/clang/lib/Tooling/Refactoring.cpp
+++ b/clang/lib/Tooling/Refactoring.cpp
@@ -78,11 +78,8 @@ bool formatAndApplyAllReplacements(
const std::string &FilePath = FileAndReplaces.first;
auto &CurReplaces = FileAndReplaces.second;
- const FileEntry *Entry = nullptr;
- if (auto File = Files.getFile(FilePath))
- Entry = *File;
-
- FileID ID = SM.getOrCreateFileID(Entry, SrcMgr::C_User);
+ OptionalFileEntryRef Entry = Files.getOptionalFileRef(FilePath);
+ FileID ID = SM.getOrCreateFileID(*Entry, SrcMgr::C_User);
StringRef Code = SM.getBufferData(ID);
auto CurStyle = format::getStyle(Style, FilePath, "LLVM");
diff --git a/clang/tools/clang-rename/ClangRename.cpp b/clang/tools/clang-rename/ClangRename.cpp
index 24c9d8521dc270f..f2ac0c4360e0dc5 100644
--- a/clang/tools/clang-rename/ClangRename.cpp
+++ b/clang/tools/clang-rename/ClangRename.cpp
@@ -228,7 +228,7 @@ int main(int argc, const char **argv) {
Tool.applyAllReplacements(Rewrite);
for (const auto &File : Files) {
- auto Entry = FileMgr.getFile(File);
+ auto Entry = FileMgr.getOptionalFileRef(File);
if (!Entry) {
errs() << "clang-rename: " << File << " does not exist.\n";
return 1;
diff --git a/clang/unittests/Analysis/UnsafeBufferUsageTest.cpp b/clang/unittests/Analysis/UnsafeBufferUsageTest.cpp
index 66af5750ef2424b..e48f39bf13f449e 100644
--- a/clang/unittests/Analysis/UnsafeBufferUsageTest.cpp
+++ b/clang/unittests/Analysis/UnsafeBufferUsageTest.cpp
@@ -25,7 +25,7 @@ class UnsafeBufferUsageTest : public ::testing::Test {
} // namespace
TEST_F(UnsafeBufferUsageTest, FixItHintsConflict) {
- const FileEntry *DummyFile = FileMgr.getVirtualFile("<virtual>", 100, 0);
+ FileEntryRef DummyFile = FileMgr.getVirtualFileRef("<virtual>", 100, 0);
FileID DummyFileID = SourceMgr.getOrCreateFileID(DummyFile, SrcMgr::C_User);
SourceLocation StartLoc = SourceMgr.getLocForStartOfFile(DummyFileID);
|
benlangmuir
reviewed
Sep 29, 2023
| @@ -152,7 +152,7 @@ int main(int argc, const char **argv) { | |||
| for (auto I = ChangedFiles.begin(), E = ChangedFiles.end(); I != E; ++I) { | |||
| OS << " {\n"; | |||
| OS << " \"FilePath\": \"" << *I << "\",\n"; | |||
| const auto Entry = FileMgr.getFile(*I); | |||
| auto Entry = FileMgr.getOptionalFileRef(*I); | |||
There was a problem hiding this comment.
In cases like this where we immediately dereference the optional, I suggest either
auto Entry = llvm::cantFail(FileMgr.getFileRef(*I));or
auto Entry = FileMgr.getFileRef(*I);
if (!Entry)
llvm::report_fatal_error(Entry.takeError);depending on whether you want to assert the value (cantFail) or want to also report the error in release builds (report_fatal_error).
The benefit here is that you get the error message if it goes wrong, instead of just a segfault.
| @@ -843,7 +843,7 @@ void ClangMoveTool::moveDeclsToNewFiles() { | |||
| // Move all contents from OldFile to NewFile. | |||
| void ClangMoveTool::moveAll(SourceManager &SM, StringRef OldFile, | |||
| StringRef NewFile) { | |||
| auto FE = SM.getFileManager().getFile(makeAbsolutePath(OldFile)); | |||
| auto FE = SM.getFileManager().getOptionalFileRef(makeAbsolutePath(OldFile)); | |||
| if (!FE) { | |||
| llvm::errs() << "Failed to get file: " << OldFile << "\n"; | |||
There was a problem hiding this comment.
Recommend getFileRef and then print the error message as well if it fails.
| @@ -527,17 +527,6 @@ FileID SourceManager::getNextFileID(FileID FID) const { | |||
|
|
|||
| /// Create a new FileID that represents the specified file | |||
| /// being \#included from the specified IncludePosition. | |||
| /// | |||
| /// This translates NULL into standard input. | |||
| FileID SourceManager::createFileID(const FileEntry *SourceFile, | |||
There was a problem hiding this comment.
Were there no callers that could pass NULL?
There was a problem hiding this comment.
Not that I could find.
| @@ -228,7 +228,7 @@ int main(int argc, const char **argv) { | |||
|
|
|||
| Tool.applyAllReplacements(Rewrite); | |||
| for (const auto &File : Files) { | |||
| auto Entry = FileMgr.getFile(File); | |||
| auto Entry = FileMgr.getOptionalFileRef(File); | |||
There was a problem hiding this comment.
Another place we could use getFileRef and print the error message.
|
Thanks for the feedback. I used |
benlangmuir
approved these changes
Oct 3, 2023
jansvoboda11
added a commit
that referenced
this pull request
Oct 3, 2023
The last usage of the deprecated `FileEntry::getLastRef()` was removed in #67838, let's remove it entirely.
swift-ci
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Oct 4, 2023
The method createFileID has been replaced with getOrCreateFileID in the following commit: commit 27254ae Author: Jan Svoboda <[email protected]> [clang] NFCI: Use FileEntryRef for FileID creation (llvm#67838)
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Oct 6, 2023
The last usage of the deprecated `FileEntry::getLastRef()` was removed in llvm#67838, let's remove it entirely. Change-Id: I690b9a38b8d9e7b4c1414d008873f3f819a27a26
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch removes the
SourceManagerAPIs that createFileIDfrom aconst FileEntry *in favor of APIs that takeFileEntryRef. This also removes a misleading documentation that claimsnullptrfile entry represents stdin. I don't think that's right, since we just try to dereference that pointer anyways.