adamcz created this revision.
adamcz added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous,
MaskRay, ilya-biryukov.
Herald added a project: clang.
sammccall accepted this revision.
This revision is now accepted and ready to land.
adamcz updated this revision to Diff 262364.
adamcz marked 2 inline comments as done.
adamcz added a comment.
Addressed review comments
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:181
+ const auto FileName = SM.getFileEntryForID(SM.getMainFileID())->getName();
+ if (FileName.endswith(".h") || FileName.endswith(".hpp")) {
+ return false;
----------------
can you use isHeaderFile instead? (in SourceCode.h)
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2471
+ EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
+ FileName = "test.hpp";
+ EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
----------------
no need for this extra test if using isHeaderFile
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2471
+ EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
+ FileName = "test.hpp";
+ EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
----------------
sammccall wrote:
> no need for this extra test if using isHeaderFile
I'd argue there is a need for this test - if only to check that we are, in
fact, using isHeaderFile() or something equivalent :-)
I can delete it if you want, but I think it's better this way.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D79488
Files:
clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2463,6 +2463,13 @@
// test that we don't crash.
EXPECT_UNAVAILABLE(Header +
"template<typename TT> using foo = one::tt<T^T>;");
+
+ // Check that we do not trigger in header files.
+ FileName = "test.h";
+ ExtraArgs.push_back("-xc++-header"); // .h file is treated a C by default.
+ EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
+ FileName = "test.hpp";
+ EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
}
TEST_F(AddUsingTest, Apply) {
Index: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -175,6 +175,12 @@
bool AddUsing::prepare(const Selection &Inputs) {
auto &SM = Inputs.AST->getSourceManager();
+
+ // Do not suggest "using" in header files. That way madness lies.
+ if (isHeaderFile(SM.getFileEntryForID(SM.getMainFileID())->getName(),
+ Inputs.AST->getLangOpts()))
+ return false;
+
auto *Node = Inputs.ASTSelection.commonAncestor();
if (Node == nullptr)
return false;
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2463,6 +2463,13 @@
// test that we don't crash.
EXPECT_UNAVAILABLE(Header +
"template<typename TT> using foo = one::tt<T^T>;");
+
+ // Check that we do not trigger in header files.
+ FileName = "test.h";
+ ExtraArgs.push_back("-xc++-header"); // .h file is treated a C by default.
+ EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
+ FileName = "test.hpp";
+ EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
}
TEST_F(AddUsingTest, Apply) {
Index: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
@@ -175,6 +175,12 @@
bool AddUsing::prepare(const Selection &Inputs) {
auto &SM = Inputs.AST->getSourceManager();
+
+ // Do not suggest "using" in header files. That way madness lies.
+ if (isHeaderFile(SM.getFileEntryForID(SM.getMainFileID())->getName(),
+ Inputs.AST->getLangOpts()))
+ return false;
+
auto *Node = Inputs.ASTSelection.commonAncestor();
if (Node == nullptr)
return false;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
