Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] NFC: Deprecate FileEntry::getName() #68157

Merged
merged 3 commits into from
Dec 6, 2023

Conversation

jansvoboda11
Copy link
Contributor

All uses of FileEntry::getName() were removed in favor of FileEntryRef::getName(). This patch finally formally deprecates that function. The plan is to remove it entirely in the main branch after we cut the release branch for LLVM 18.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 3, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 3, 2023

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-clang

Changes

All uses of FileEntry::getName() were removed in favor of FileEntryRef::getName(). This patch finally formally deprecates that function. The plan is to remove it entirely in the main branch after we cut the release branch for LLVM 18.


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

2 Files Affected:

  • (modified) clang/include/clang/Basic/FileEntry.h (+1)
  • (modified) clang/unittests/Basic/FileManagerTest.cpp (-2)
diff --git a/clang/include/clang/Basic/FileEntry.h b/clang/include/clang/Basic/FileEntry.h
index 23237a9326f84b6..e838c9e27b6370a 100644
--- a/clang/include/clang/Basic/FileEntry.h
+++ b/clang/include/clang/Basic/FileEntry.h
@@ -425,6 +425,7 @@ class FileEntry {
 
 public:
   ~FileEntry();
+  LLVM_DEPRECATED("Use FileEntryRef::getName() instead.", "")
   StringRef getName() const { return LastRef->getName(); }
 
   StringRef tryGetRealPathName() const { return RealPathName; }
diff --git a/clang/unittests/Basic/FileManagerTest.cpp b/clang/unittests/Basic/FileManagerTest.cpp
index bf30fabb7cd8878..d32036d975ce9cf 100644
--- a/clang/unittests/Basic/FileManagerTest.cpp
+++ b/clang/unittests/Basic/FileManagerTest.cpp
@@ -284,7 +284,6 @@ TEST_F(FileManagerTest, getFileRefReturnsCorrectNameForDifferentStatPath) {
   ASSERT_FALSE(!F1Alias);
   ASSERT_FALSE(!F1Alias2);
   EXPECT_EQ("dir/f1.cpp", F1->getName());
-  EXPECT_EQ("dir/f1.cpp", F1->getFileEntry().getName());
   EXPECT_EQ("dir/f1.cpp", F1Alias->getName());
   EXPECT_EQ("dir/f1.cpp", F1Alias2->getName());
   EXPECT_EQ(&F1->getFileEntry(), &F1Alias->getFileEntry());
@@ -303,7 +302,6 @@ TEST_F(FileManagerTest, getFileRefReturnsCorrectNameForDifferentStatPath) {
   ASSERT_FALSE(!F2Alias);
   ASSERT_FALSE(!F2Alias2);
   EXPECT_EQ("dir/f2.cpp", F2->getName());
-  EXPECT_EQ("dir/f2.cpp", F2->getFileEntry().getName());
   EXPECT_EQ("dir/f2.cpp", F2Alias->getName());
   EXPECT_EQ("dir/f2.cpp", F2Alias2->getName());
   EXPECT_EQ(&F2->getFileEntry(), &F2Alias->getFileEntry());

Copy link
Collaborator

@benlangmuir benlangmuir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

clang/unittests/Basic/FileManagerTest.cpp Show resolved Hide resolved
@bnbarham
Copy link
Contributor

bnbarham commented Dec 4, 2023

🎉 thanks @jansvoboda11 for slogging through all these!

@@ -157,6 +157,25 @@
#define LLVM_DEPRECATED(MSG, FIX) [[deprecated(MSG)]]
#endif

// clang-format off
#if defined(__clang__) || defined(__GNUC__)
#define LLVM_IGNORE_DEPRECATIONS_OF_DECLARATIONS_BEGIN \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could have a more generic macro?

#define DO_PRAGMA(x) _Pragma(#x)
#if defined(__clang__) || defined(__GNUC__)
#define LLVM_DISABLE_WARNING_BEGIN(GCC_FLAG, MSVC_NUMBER) \
  DO_PRAGMA(GCC diagnostic push)                          \
  DO_PRAGMA(GCC diagnostic ignored "-W" GCC_FLAG)
#define LLVM_DISABLE_WARNING_END                          \
  DO_PRAGMA(GCC diagnostic pop)
#elif defined(_MSC_VER)
#define LLVM_DISABLE_WARNING_BEGIN(GCC_FLAG, MSVC_NUMBER) \
  DO_PRAGMA(warning(push))                                \
  DO_PRAGMA(warning(disable: MSVC_NUMBER))
#define LLVM_DISABLE_WARNING_END                          \
  DO_PRAGMA(warning(pop))
#else
#define LLVM_DISABLE_WARNING_BEGIN(GCC_FLAG, MSVC_NUMBER)
#define LLVM_DISABLE_WARNING_END
#endif

LLVM_DISABLE_WARNING_BEGIN("deprecated-declarations", 4996)
EXPECT_EQ("dir/f2.cpp", F2->getFileEntry().getName());
LLVM_DISABLE_WARNING_END

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how often the general form of this is useful - I don't actually see many places in the code that disable diagnostics for both MSVC and clang/gcc. So I would stick with the version that is specific to deprecations. Maybe you should more closely match the name of _LIBCPP_SUPPRESS_DEPRECATED_PUSH -- e.g. LLVM_SUPPRESS_DEPRECATED_PUSH and ..._POP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I used the suggested naming, but added DECLARATIONS to distinguish from semantics of the libc++ version that also ignores -Wdeprecated, which I don't want to (and I'm not sure there's an MSVC counterpart). I left this in Compiler.h, since this might be useful in other parts of LLVM and for downstream projects that have extra usages of FileEntry::getName().

@jansvoboda11 jansvoboda11 merged commit d1f86c3 into llvm:main Dec 6, 2023
3 of 4 checks passed
@jansvoboda11 jansvoboda11 deleted the deprecate-get-name branch December 6, 2023 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category llvm:support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants