[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
This revision was automatically updated to reflect the committed changes. Closed by commit rG1b66840f7103: [clangd][ObjC] Support ObjC class rename from implementation decls (authored by dgoldman). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 Files: clang-tools-extra/clangd/FindTarget.cpp clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/refactor/Rename.cpp clang-tools-extra/clangd/unittests/FindTargetTests.cpp clang-tools-extra/clangd/unittests/RenameTests.cpp Index: clang-tools-extra/clangd/unittests/RenameTests.cpp === --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -840,6 +840,20 @@ foo('x'); } )cpp", + + // ObjC class with a category. + R"cpp( +@interface [[Fo^o]] +@end +@implementation [[F^oo]] +@end +@interface [[Fo^o]] (Category) +@end +@implementation [[F^oo]] (Category) +@end + +void func([[Fo^o]] *f) {} + )cpp", }; llvm::StringRef NewName = "NewName"; for (llvm::StringRef T : Tests) { @@ -890,6 +904,15 @@ )cpp", "not a supported kind", HeaderFile}, + {R"cpp(// disallow - category rename. +@interface Foo +@end +@interface Foo (Cate^gory) +@end + )cpp", + "Cannot rename symbol: there is no symbol at the given location", + HeaderFile}, + { R"cpp( #define MACRO 1 @@ -1468,7 +1491,7 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) { MockCompilationDatabase CDB; - CDB.ExtraClangFlags = {"-xc++"}; + CDB.ExtraClangFlags = {"-xobjective-c++"}; // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the // expected rename occurrences. struct Case { @@ -1557,13 +1580,12 @@ } )cpp", }, - { - // virtual templated method - R"cpp( + {// virtual templated method + R"cpp( template class Foo { virtual void [[m]](); }; class Bar : Foo { void [[^m]]() override; }; )cpp", - R"cpp( + R"cpp( #include "foo.h" template void Foo::[[m]]() {} @@ -1571,8 +1593,7 @@ // the canonical Foo::m(). // https://github.com/clangd/clangd/issues/1325 class Baz : Foo { void m() override; }; -)cpp" - }, +)cpp"}, { // rename on constructor and destructor. R"cpp( @@ -1677,6 +1698,20 @@ } )cpp", }, + { + // Objective-C classes. + R"cpp( +@interface [[Fo^o]] +@end + )cpp", + R"cpp( +#include "foo.h" +@implementation [[Foo]] +@end + +void func([[Foo]] *f) {} + )cpp", + }, }; trace::TestTracer Tracer; Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp === --- clang-tools-extra/clangd/unittests/FindTargetTests.cpp +++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp @@ -1127,6 +1127,16 @@ )cpp"; EXPECT_DECLS("ObjCCategoryImplDecl", "@interface Foo(Ext)"); + Code = R"cpp( +@interface Foo +@end +@interface Foo (Ext) +@end +@implementation Foo ([[Ext]]) +@end + )cpp"; + EXPECT_DECLS("ObjCCategoryImplDecl", "@interface Foo(Ext)"); + Code = R"cpp( void test(id p); )cpp"; @@ -1216,10 +1226,7 @@ std::string DumpedReferences; }; - /// Parses \p Code, finds function or namespace '::foo' and annotates its body - /// with results of findExplicitReferences. - /// See actual tests for examples of annotation format. - AllRefs annotateReferencesInFoo(llvm::StringRef Code) { + TestTU newTU(llvm::StringRef Code) { TestTU TU; TU.Code = std::string(Code); @@ -1228,30 +1235,11 @@ TU.ExtraArgs.push_back("-std=c++20"); TU.ExtraArgs.push_back("-xobjective-c++"); -auto AST = TU.build(); -auto *TestDecl = &findDecl(AST, "foo"); -if (auto *T = llvm::dyn_cast(TestDecl)) - TestDecl = T->getTemplatedDecl(); - -std::vector Refs; -if (const auto *Func = llvm::dyn_cast(TestDecl)) - findExplicitReferences( - Func->getBody(), - [&Refs](ReferenceLoc R) { Refs.push_back(std::move(R)); }, - AST.getHeuristicResolver()); -else if (const auto *NS = llvm::dyn_cast(TestDecl)) - findExplicitReferences( - NS, - [&Refs, &NS](ReferenceLoc R) { -// Avoid adding the namespace foo decl to the results. -if (R.Targets.size() == 1 && R.Targets.front() == NS) - return; -Refs.push_back(std::move(R)); - }, - AST.getHeuristicResolver()); -else - ADD
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
dgoldman updated this revision to Diff 534618. dgoldman marked an inline comment as done. dgoldman added a comment. Fixes for review + rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 Files: clang-tools-extra/clangd/FindTarget.cpp clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/refactor/Rename.cpp clang-tools-extra/clangd/unittests/FindTargetTests.cpp clang-tools-extra/clangd/unittests/RenameTests.cpp Index: clang-tools-extra/clangd/unittests/RenameTests.cpp === --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -840,6 +840,20 @@ foo('x'); } )cpp", + + // ObjC class with a category. + R"cpp( +@interface [[Fo^o]] +@end +@implementation [[F^oo]] +@end +@interface [[Fo^o]] (Category) +@end +@implementation [[F^oo]] (Category) +@end + +void func([[Fo^o]] *f) {} + )cpp", }; llvm::StringRef NewName = "NewName"; for (llvm::StringRef T : Tests) { @@ -890,6 +904,15 @@ )cpp", "not a supported kind", HeaderFile}, + {R"cpp(// disallow - category rename. +@interface Foo +@end +@interface Foo (Cate^gory) +@end + )cpp", + "Cannot rename symbol: there is no symbol at the given location", + HeaderFile}, + { R"cpp( #define MACRO 1 @@ -1468,7 +1491,7 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) { MockCompilationDatabase CDB; - CDB.ExtraClangFlags = {"-xc++"}; + CDB.ExtraClangFlags = {"-xobjective-c++"}; // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the // expected rename occurrences. struct Case { @@ -1557,13 +1580,12 @@ } )cpp", }, - { - // virtual templated method - R"cpp( + {// virtual templated method + R"cpp( template class Foo { virtual void [[m]](); }; class Bar : Foo { void [[^m]]() override; }; )cpp", - R"cpp( + R"cpp( #include "foo.h" template void Foo::[[m]]() {} @@ -1571,8 +1593,7 @@ // the canonical Foo::m(). // https://github.com/clangd/clangd/issues/1325 class Baz : Foo { void m() override; }; -)cpp" - }, +)cpp"}, { // rename on constructor and destructor. R"cpp( @@ -1677,6 +1698,20 @@ } )cpp", }, + { + // Objective-C classes. + R"cpp( +@interface [[Fo^o]] +@end + )cpp", + R"cpp( +#include "foo.h" +@implementation [[Foo]] +@end + +void func([[Foo]] *f) {} + )cpp", + }, }; trace::TestTracer Tracer; Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp === --- clang-tools-extra/clangd/unittests/FindTargetTests.cpp +++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp @@ -1127,6 +1127,16 @@ )cpp"; EXPECT_DECLS("ObjCCategoryImplDecl", "@interface Foo(Ext)"); + Code = R"cpp( +@interface Foo +@end +@interface Foo (Ext) +@end +@implementation Foo ([[Ext]]) +@end + )cpp"; + EXPECT_DECLS("ObjCCategoryImplDecl", "@interface Foo(Ext)"); + Code = R"cpp( void test(id p); )cpp"; @@ -1216,10 +1226,7 @@ std::string DumpedReferences; }; - /// Parses \p Code, finds function or namespace '::foo' and annotates its body - /// with results of findExplicitReferences. - /// See actual tests for examples of annotation format. - AllRefs annotateReferencesInFoo(llvm::StringRef Code) { + TestTU newTU(llvm::StringRef Code) { TestTU TU; TU.Code = std::string(Code); @@ -1228,30 +1235,11 @@ TU.ExtraArgs.push_back("-std=c++20"); TU.ExtraArgs.push_back("-xobjective-c++"); -auto AST = TU.build(); -auto *TestDecl = &findDecl(AST, "foo"); -if (auto *T = llvm::dyn_cast(TestDecl)) - TestDecl = T->getTemplatedDecl(); - -std::vector Refs; -if (const auto *Func = llvm::dyn_cast(TestDecl)) - findExplicitReferences( - Func->getBody(), - [&Refs](ReferenceLoc R) { Refs.push_back(std::move(R)); }, - AST.getHeuristicResolver()); -else if (const auto *NS = llvm::dyn_cast(TestDecl)) - findExplicitReferences( - NS, - [&Refs, &NS](ReferenceLoc R) { -// Avoid adding the namespace foo decl to the results. -if (R.Targets.size() == 1 && R.Targets.front() == NS) - return; -Refs.push_back(std::move(R)); - }, - AST.getHeuristicResolver()); -else - ADD_FAILURE() << "Failed to find ::foo decl for test"; +
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks! Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:144 +const NamedDecl *pickInterestingTarget(const NamedDecl *D) { + // We only support renaming the class name, not the category name. This has can you put a function level comment like: ``` Some AST nodes can reference multiple declarations. We try to pick the relevant one to rename here. ``` Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:1709 +#include "foo.h" +@implementation [[F^oo]] +@end can you drop the `^` in here. we only iterate over points in the header file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
dgoldman added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:787 + // names like class and protocol names. + if (const auto *CD = dyn_cast(&RenameDecl)) +if (CD->getName() != IdentifierToken->text(SM)) kadircet wrote: > this special casing should no longer be needed if we just map CategoryDecls > to InterfaceDecls in `locateDeclAt` rather than at canonicalization time Yep, no longer needed since the `Check the rename-triggering location is actually being renamed` check catches this although the error is no symbol found instead of unsupported symbol. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
dgoldman updated this revision to Diff 533993. dgoldman marked 4 inline comments as done. dgoldman added a comment. Fixes for review Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 Files: clang-tools-extra/clangd/FindTarget.cpp clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/refactor/Rename.cpp clang-tools-extra/clangd/unittests/FindTargetTests.cpp clang-tools-extra/clangd/unittests/RenameTests.cpp Index: clang-tools-extra/clangd/unittests/RenameTests.cpp === --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -840,6 +840,20 @@ foo('x'); } )cpp", + + // ObjC class with a category. + R"cpp( +@interface [[Fo^o]] +@end +@implementation [[F^oo]] +@end +@interface [[Fo^o]] (Category) +@end +@implementation [[F^oo]] (Category) +@end + +void func([[Fo^o]] *f) {} + )cpp", }; llvm::StringRef NewName = "NewName"; for (llvm::StringRef T : Tests) { @@ -890,6 +904,15 @@ )cpp", "not a supported kind", HeaderFile}, + {R"cpp(// disallow - category rename. +@interface Foo +@end +@interface Foo (Cate^gory) +@end + )cpp", + "Cannot rename symbol: there is no symbol at the given location", + HeaderFile}, + { R"cpp( #define MACRO 1 @@ -1468,7 +1491,7 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) { MockCompilationDatabase CDB; - CDB.ExtraClangFlags = {"-xc++"}; + CDB.ExtraClangFlags = {"-xobjective-c++"}; // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the // expected rename occurrences. struct Case { @@ -1557,13 +1580,12 @@ } )cpp", }, - { - // virtual templated method - R"cpp( + {// virtual templated method + R"cpp( template class Foo { virtual void [[m]](); }; class Bar : Foo { void [[^m]]() override; }; )cpp", - R"cpp( + R"cpp( #include "foo.h" template void Foo::[[m]]() {} @@ -1571,8 +1593,7 @@ // the canonical Foo::m(). // https://github.com/clangd/clangd/issues/1325 class Baz : Foo { void m() override; }; -)cpp" - }, +)cpp"}, { // rename on constructor and destructor. R"cpp( @@ -1677,6 +1698,20 @@ } )cpp", }, + { + // Objective-C classes. + R"cpp( +@interface [[Fo^o]] +@end + )cpp", + R"cpp( +#include "foo.h" +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", + }, }; trace::TestTracer Tracer; Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp === --- clang-tools-extra/clangd/unittests/FindTargetTests.cpp +++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp @@ -1054,6 +1054,16 @@ )cpp"; EXPECT_DECLS("ObjCCategoryImplDecl", "@interface Foo(Ext)"); + Code = R"cpp( +@interface Foo +@end +@interface Foo (Ext) +@end +@implementation Foo ([[Ext]]) +@end + )cpp"; + EXPECT_DECLS("ObjCCategoryImplDecl", "@interface Foo(Ext)"); + Code = R"cpp( void test(id p); )cpp"; @@ -1143,10 +1153,7 @@ std::string DumpedReferences; }; - /// Parses \p Code, finds function or namespace '::foo' and annotates its body - /// with results of findExplicitReferences. - /// See actual tests for examples of annotation format. - AllRefs annotateReferencesInFoo(llvm::StringRef Code) { + TestTU newTU(llvm::StringRef Code) { TestTU TU; TU.Code = std::string(Code); @@ -1155,30 +1162,11 @@ TU.ExtraArgs.push_back("-std=c++20"); TU.ExtraArgs.push_back("-xobjective-c++"); -auto AST = TU.build(); -auto *TestDecl = &findDecl(AST, "foo"); -if (auto *T = llvm::dyn_cast(TestDecl)) - TestDecl = T->getTemplatedDecl(); - -std::vector Refs; -if (const auto *Func = llvm::dyn_cast(TestDecl)) - findExplicitReferences( - Func->getBody(), - [&Refs](ReferenceLoc R) { Refs.push_back(std::move(R)); }, - AST.getHeuristicResolver()); -else if (const auto *NS = llvm::dyn_cast(TestDecl)) - findExplicitReferences( - NS, - [&Refs, &NS](ReferenceLoc R) { -// Avoid adding the namespace foo decl to the results. -if (R.Targets.size() == 1 && R.Targets.front() == NS) - return; -Refs.push_back(std::move(R)); - }, - AST.getHeuristicResolver()); -else - ADD_FAILURE() << "Failed to find ::foo decl for test"; +return
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
kadircet added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:164 AST.getHeuristicResolver())) { Result.insert(canonicalRenameDecl(D)); } before calling `canonicalRenameDecl` here we can do a `D = pickInterestingTarget(D);` and map it to interface decl once (we'll probably need something similar for propertydecls?), rather than canonicalizing all ObjcCategoryDecls. that way we'll still get to rename proper references that target ObjcInterfaceDecls. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:288 +// the interface(s) which they reference. +if (isa(Target)) + continue; i am afraid we're doing this at the wrong layer. the discrepancy is actually arising from the fact that `findExplicitReferences` will report name locations for an entity whose primary location contains a different name (e.g. `@interface ^Foo (^Bar)` saying that there's a reference to objccategorydecl at `Bar`, but canonicalizing it to `Foo`). so i think what we really want is not to canonicalize objccategory(impl)decls to objcinterfacedecl, but rather pick rename target as objcinterfacedecl when the user is trying to rename a categorydecl inside `locatelDeclAt` (put more comments near that part of the code). sorry for the extra round trip here :/ Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:787 + // names like class and protocol names. + if (const auto *CD = dyn_cast(&RenameDecl)) +if (CD->getName() != IdentifierToken->text(SM)) this special casing should no longer be needed if we just map CategoryDecls to InterfaceDecls in `locateDeclAt` rather than at canonicalization time Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:1990 + + std::pair + AllRefsCases[] = {// Simple expressions. can you move this into a separate test case? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
dgoldman marked an inline comment as done. dgoldman added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:716 +void VisitObjCImplementationDecl(const ObjCImplementationDecl *OIMD) { + if (const auto *CI = OIMD->getClassInterface()) +Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(), kadircet wrote: > we don't have the null check in other places, what's the significance here? Removed, doesn't look like it's needed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
dgoldman updated this revision to Diff 533769. dgoldman marked an inline comment as done. dgoldman added a comment. Add more FindTarget tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 Files: clang-tools-extra/clangd/FindTarget.cpp clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/refactor/Rename.cpp clang-tools-extra/clangd/unittests/FindTargetTests.cpp clang-tools-extra/clangd/unittests/RenameTests.cpp Index: clang-tools-extra/clangd/unittests/RenameTests.cpp === --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -840,6 +840,20 @@ foo('x'); } )cpp", + + // ObjC class with a category. + R"cpp( +@interface [[Fo^o]] +@end +@implementation [[F^oo]] +@end +@interface [[Fo^o]] (Category) +@end +@implementation [[F^oo]] (Category) +@end + +void func([[Fo^o]] *f) {} + )cpp", }; llvm::StringRef NewName = "NewName"; for (llvm::StringRef T : Tests) { @@ -890,6 +904,14 @@ )cpp", "not a supported kind", HeaderFile}, + {R"cpp(// disallow - category rename. +@interface Foo +@end +@interface Foo (Cate^gory) +@end + )cpp", + "not a supported kind", HeaderFile}, + { R"cpp( #define MACRO 1 @@ -1468,7 +1490,7 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) { MockCompilationDatabase CDB; - CDB.ExtraClangFlags = {"-xc++"}; + CDB.ExtraClangFlags = {"-xobjective-c++"}; // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the // expected rename occurrences. struct Case { @@ -1557,13 +1579,12 @@ } )cpp", }, - { - // virtual templated method - R"cpp( + {// virtual templated method + R"cpp( template class Foo { virtual void [[m]](); }; class Bar : Foo { void [[^m]]() override; }; )cpp", - R"cpp( + R"cpp( #include "foo.h" template void Foo::[[m]]() {} @@ -1571,8 +1592,7 @@ // the canonical Foo::m(). // https://github.com/clangd/clangd/issues/1325 class Baz : Foo { void m() override; }; -)cpp" - }, +)cpp"}, { // rename on constructor and destructor. R"cpp( @@ -1677,6 +1697,20 @@ } )cpp", }, + { + // Objective-C classes. + R"cpp( +@interface [[Fo^o]] +@end + )cpp", + R"cpp( +#include "foo.h" +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", + }, }; trace::TestTracer Tracer; Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp === --- clang-tools-extra/clangd/unittests/FindTargetTests.cpp +++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp @@ -1054,6 +1054,16 @@ )cpp"; EXPECT_DECLS("ObjCCategoryImplDecl", "@interface Foo(Ext)"); + Code = R"cpp( +@interface Foo +@end +@interface Foo (Ext) +@end +@implementation Foo ([[Ext]]) +@end + )cpp"; + EXPECT_DECLS("ObjCCategoryImplDecl", "@interface Foo(Ext)"); + Code = R"cpp( void test(id p); )cpp"; @@ -1143,10 +1153,7 @@ std::string DumpedReferences; }; - /// Parses \p Code, finds function or namespace '::foo' and annotates its body - /// with results of findExplicitReferences. - /// See actual tests for examples of annotation format. - AllRefs annotateReferencesInFoo(llvm::StringRef Code) { + TestTU newTU(llvm::StringRef Code) { TestTU TU; TU.Code = std::string(Code); @@ -1155,30 +1162,11 @@ TU.ExtraArgs.push_back("-std=c++20"); TU.ExtraArgs.push_back("-xobjective-c++"); -auto AST = TU.build(); -auto *TestDecl = &findDecl(AST, "foo"); -if (auto *T = llvm::dyn_cast(TestDecl)) - TestDecl = T->getTemplatedDecl(); - -std::vector Refs; -if (const auto *Func = llvm::dyn_cast(TestDecl)) - findExplicitReferences( - Func->getBody(), - [&Refs](ReferenceLoc R) { Refs.push_back(std::move(R)); }, - AST.getHeuristicResolver()); -else if (const auto *NS = llvm::dyn_cast(TestDecl)) - findExplicitReferences( - NS, - [&Refs, &NS](ReferenceLoc R) { -// Avoid adding the namespace foo decl to the results. -if (R.Targets.size() == 1 && R.Targets.front() == NS) - return; -Refs.push_back(std::move(R)); - }, - AST.getHeuristicResolver()); -else - ADD_FAILURE() << "Failed to find ::foo decl for test"; +return TU; + } + AllRefs annotatedReference
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
dgoldman updated this revision to Diff 533640. dgoldman added a comment. Disable renaming categories Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 Files: clang-tools-extra/clangd/FindTarget.cpp clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/refactor/Rename.cpp clang-tools-extra/clangd/unittests/RenameTests.cpp Index: clang-tools-extra/clangd/unittests/RenameTests.cpp === --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -840,6 +840,20 @@ foo('x'); } )cpp", + + // ObjC class with a category. + R"cpp( +@interface [[Fo^o]] +@end +@implementation [[F^oo]] +@end +@interface [[Fo^o]] (Category) +@end +@implementation [[F^oo]] (Category) +@end + +void func([[Fo^o]] *f) {} + )cpp", }; llvm::StringRef NewName = "NewName"; for (llvm::StringRef T : Tests) { @@ -890,6 +904,14 @@ )cpp", "not a supported kind", HeaderFile}, + {R"cpp(// disallow - category rename. +@interface Foo +@end +@interface Foo (Cate^gory) +@end + )cpp", + "not a supported kind", HeaderFile}, + { R"cpp( #define MACRO 1 @@ -1468,7 +1490,7 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) { MockCompilationDatabase CDB; - CDB.ExtraClangFlags = {"-xc++"}; + CDB.ExtraClangFlags = {"-xobjective-c++"}; // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the // expected rename occurrences. struct Case { @@ -1557,13 +1579,12 @@ } )cpp", }, - { - // virtual templated method - R"cpp( + {// virtual templated method + R"cpp( template class Foo { virtual void [[m]](); }; class Bar : Foo { void [[^m]]() override; }; )cpp", - R"cpp( + R"cpp( #include "foo.h" template void Foo::[[m]]() {} @@ -1571,8 +1592,7 @@ // the canonical Foo::m(). // https://github.com/clangd/clangd/issues/1325 class Baz : Foo { void m() override; }; -)cpp" - }, +)cpp"}, { // rename on constructor and destructor. R"cpp( @@ -1677,6 +1697,20 @@ } )cpp", }, + { + // Objective-C classes. + R"cpp( +@interface [[Fo^o]] +@end + )cpp", + R"cpp( +#include "foo.h" +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", + }, }; trace::TestTracer Tracer; Index: clang-tools-extra/clangd/refactor/Rename.cpp === --- clang-tools-extra/clangd/refactor/Rename.cpp +++ clang-tools-extra/clangd/refactor/Rename.cpp @@ -19,6 +19,7 @@ #include "clang/AST/ASTTypeTraits.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" +#include "clang/AST/DeclObjC.h" #include "clang/AST/DeclTemplate.h" #include "clang/AST/ParentMapContext.h" #include "clang/AST/Stmt.h" @@ -137,6 +138,10 @@ if (const auto *TargetDecl = UD->getTargetDecl()) return canonicalRenameDecl(TargetDecl); } + if (const auto *CD = dyn_cast(D)) { +if (const auto CI = CD->getClassInterface()) + return canonicalRenameDecl(CI); + } return dyn_cast(D->getCanonicalDecl()); } @@ -278,6 +283,10 @@ if (Ref.Targets.empty()) return; for (const auto *Target : Ref.Targets) { +// Skip categories since we don't support renaming them, only +// the interface(s) which they reference. +if (isa(Target)) + continue; if (canonicalRenameDecl(Target) == &ND) { Results.push_back(Ref.NameLoc); return; @@ -773,6 +782,12 @@ if (Invalid) return makeError(std::move(*Invalid)); + // We don't support renaming the category name, only ObjC top level container + // names like class and protocol names. + if (const auto *CD = dyn_cast(&RenameDecl)) +if (CD->getName() != IdentifierToken->text(SM)) + return makeError(ReasonToReject::UnsupportedSymbol); + auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index, Opts); if (Reject) Index: clang-tools-extra/clangd/SemanticHighlighting.cpp === --- clang-tools-extra/clangd/SemanticHighlighting.cpp +++ clang-tools-extra/clangd/SemanticHighlighting.cpp @@ -128,7 +128,7 @@ return HighlightingKind::Class; if (isa(D)) return HighlightingKind::Interface; - if (isa(D)) + if (isa(D)) return HighlightingKind::Namespace; if (auto *MD = dyn_cast(D))
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
dgoldman added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:131 return HighlightingKind::Interface; - if (isa(D)) + if (isa(D)) return HighlightingKind::Namespace; kadircet wrote: > let's do this in a separate change, with some tests Given the other changes, this is needed otherwise the semantic highlighting test fails. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:171-177 +if (const auto *C = dyn_cast(D)) { + if (C->getLocation() == TokenStartLoc) +D = C->getClassInterface(); + else if (const auto *I = C->getImplementation()) +if (I->getLocation() == TokenStartLoc) + D = C->getClassInterface(); +} kadircet wrote: > sorry i don't follow what's the logic doing here and we're likely doing these > in the wrong layer. > > we should either: > - Fix selection tree to pick the correct ASTNode, if it's picking the wrong > one due to not having special cases for these locations here > - Fix the `targetDecl` logic to make sure it emits all the declarations that > might be referenced by this ast node, if it's missing ClassInterface. > - Fix the canonicalRenameDecl, if we should always prefer `ClassInterface` in > these cases. Replied to you in chat, LMK what you think. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
kadircet added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:715 + +void VisitObjCImplementationDecl(const ObjCImplementationDecl *OIMD) { + if (const auto *CI = OIMD->getClassInterface()) can you also add tests for these into `FindTargetTests`? Comment at: clang-tools-extra/clangd/FindTarget.cpp:716 +void VisitObjCImplementationDecl(const ObjCImplementationDecl *OIMD) { + if (const auto *CI = OIMD->getClassInterface()) +Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(), we don't have the null check in other places, what's the significance here? Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:131 return HighlightingKind::Interface; - if (isa(D)) + if (isa(D)) return HighlightingKind::Namespace; let's do this in a separate change, with some tests Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:171-177 +if (const auto *C = dyn_cast(D)) { + if (C->getLocation() == TokenStartLoc) +D = C->getClassInterface(); + else if (const auto *I = C->getImplementation()) +if (I->getLocation() == TokenStartLoc) + D = C->getClassInterface(); +} sorry i don't follow what's the logic doing here and we're likely doing these in the wrong layer. we should either: - Fix selection tree to pick the correct ASTNode, if it's picking the wrong one due to not having special cases for these locations here - Fix the `targetDecl` logic to make sure it emits all the declarations that might be referenced by this ast node, if it's missing ClassInterface. - Fix the canonicalRenameDecl, if we should always prefer `ClassInterface` in these cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
dgoldman updated this revision to Diff 531439. dgoldman added a comment. - Implement discussed fixes + category support Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 Files: clang-tools-extra/clangd/FindTarget.cpp clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/refactor/Rename.cpp clang-tools-extra/clangd/unittests/RenameTests.cpp Index: clang-tools-extra/clangd/unittests/RenameTests.cpp === --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -840,6 +840,30 @@ foo('x'); } )cpp", + + // ObjC class with a category. + R"cpp( +@interface [[Fo^o]] +@end +@implementation [[F^oo]] +@end +@interface [[Fo^o]] (Category) +@end +@implementation [[F^oo]] (Category) +@end + +void func([[Fo^o]] *f) {} + )cpp", + + // ObjC category. + R"cpp( +@interface Foo +@end +@interface Foo ([[Cate^gory]]) +@end +@implementation Foo ([[Cate^gory]]) +@end + )cpp", }; llvm::StringRef NewName = "NewName"; for (llvm::StringRef T : Tests) { @@ -1468,7 +1492,7 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) { MockCompilationDatabase CDB; - CDB.ExtraClangFlags = {"-xc++"}; + CDB.ExtraClangFlags = {"-xobjective-c++"}; // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the // expected rename occurrences. struct Case { @@ -1557,13 +1581,12 @@ } )cpp", }, - { - // virtual templated method - R"cpp( + {// virtual templated method + R"cpp( template class Foo { virtual void [[m]](); }; class Bar : Foo { void [[^m]]() override; }; )cpp", - R"cpp( + R"cpp( #include "foo.h" template void Foo::[[m]]() {} @@ -1571,8 +1594,7 @@ // the canonical Foo::m(). // https://github.com/clangd/clangd/issues/1325 class Baz : Foo { void m() override; }; -)cpp" - }, +)cpp"}, { // rename on constructor and destructor. R"cpp( @@ -1677,6 +1699,20 @@ } )cpp", }, + { + // Objective-C classes. + R"cpp( +@interface [[Fo^o]] +@end + )cpp", + R"cpp( +#include "foo.h" +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", + }, }; trace::TestTracer Tracer; Index: clang-tools-extra/clangd/refactor/Rename.cpp === --- clang-tools-extra/clangd/refactor/Rename.cpp +++ clang-tools-extra/clangd/refactor/Rename.cpp @@ -19,6 +19,7 @@ #include "clang/AST/ASTTypeTraits.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" +#include "clang/AST/DeclObjC.h" #include "clang/AST/DeclTemplate.h" #include "clang/AST/ParentMapContext.h" #include "clang/AST/Stmt.h" @@ -137,6 +138,14 @@ if (const auto *TargetDecl = UD->getTargetDecl()) return canonicalRenameDecl(TargetDecl); } + if (const auto *ID = dyn_cast(D)) { +if (const auto CI = ID->getClassInterface()) + return canonicalRenameDecl(CI); + } + if (const auto *CID = dyn_cast(D)) { +if (const auto CD = CID->getCategoryDecl()) + return canonicalRenameDecl(CD); + } return dyn_cast(D->getCanonicalDecl()); } @@ -156,6 +165,16 @@ targetDecl(SelectedNode->ASTNode, DeclRelation::Alias | DeclRelation::TemplatePattern, AST.getHeuristicResolver())) { +// If we select the interface name in `@interface Class (CategoryName)` or +// the implementation, the decl to rename is actually the interface, not +// the category. +if (const auto *C = dyn_cast(D)) { + if (C->getLocation() == TokenStartLoc) +D = C->getClassInterface(); + else if (const auto *I = C->getImplementation()) +if (I->getLocation() == TokenStartLoc) + D = C->getClassInterface(); +} Result.insert(canonicalRenameDecl(D)); } return Result; Index: clang-tools-extra/clangd/SemanticHighlighting.cpp === --- clang-tools-extra/clangd/SemanticHighlighting.cpp +++ clang-tools-extra/clangd/SemanticHighlighting.cpp @@ -128,7 +128,7 @@ return HighlightingKind::Class; if (isa(D)) return HighlightingKind::Interface; - if (isa(D)) + if (isa(D)) return HighlightingKind::Namespace; if (auto *MD = dyn_cast(D)) return MD->isStatic() ? HighlightingKind::StaticMethod Index: clang-tools-extra/clangd/FindTarget.cpp === --- clang-
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
dgoldman updated this revision to Diff 530665. dgoldman added a comment. Run clang format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152720/new/ https://reviews.llvm.org/D152720 Files: clang-tools-extra/clangd/FindTarget.cpp clang-tools-extra/clangd/unittests/RenameTests.cpp Index: clang-tools-extra/clangd/unittests/RenameTests.cpp === --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -840,6 +840,16 @@ foo('x'); } )cpp", + + // ObjC class. + R"cpp( +@interface [[Fo^o]] +@end +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", }; llvm::StringRef NewName = "NewName"; for (llvm::StringRef T : Tests) { @@ -1468,7 +1478,7 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) { MockCompilationDatabase CDB; - CDB.ExtraClangFlags = {"-xc++"}; + CDB.ExtraClangFlags = {"-xobjective-c++"}; // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the // expected rename occurrences. struct Case { @@ -1557,13 +1567,12 @@ } )cpp", }, - { - // virtual templated method - R"cpp( + {// virtual templated method + R"cpp( template class Foo { virtual void [[m]](); }; class Bar : Foo { void [[^m]]() override; }; )cpp", - R"cpp( + R"cpp( #include "foo.h" template void Foo::[[m]]() {} @@ -1571,8 +1580,7 @@ // the canonical Foo::m(). // https://github.com/clangd/clangd/issues/1325 class Baz : Foo { void m() override; }; -)cpp" - }, +)cpp"}, { // rename on constructor and destructor. R"cpp( @@ -1677,6 +1685,20 @@ } )cpp", }, + { + // Objective-C classes. + R"cpp( +@interface [[Fo^o]] +@end + )cpp", + R"cpp( +#include "foo.h" +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", + }, }; trace::TestTracer Tracer; Index: clang-tools-extra/clangd/FindTarget.cpp === --- clang-tools-extra/clangd/FindTarget.cpp +++ clang-tools-extra/clangd/FindTarget.cpp @@ -707,6 +707,14 @@ /*IsDecl=*/true, {OCID->getCategoryDecl()}}); } + +void VisitObjCImplementationDecl(const ObjCImplementationDecl *OIMD) { + if (const auto *CI = OIMD->getClassInterface()) +Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(), +OIMD->getLocation(), +/*IsDecl=*/true, +{CI}}); +} }; Visitor V{Resolver}; Index: clang-tools-extra/clangd/unittests/RenameTests.cpp === --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -840,6 +840,16 @@ foo('x'); } )cpp", + + // ObjC class. + R"cpp( +@interface [[Fo^o]] +@end +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", }; llvm::StringRef NewName = "NewName"; for (llvm::StringRef T : Tests) { @@ -1468,7 +1478,7 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) { MockCompilationDatabase CDB; - CDB.ExtraClangFlags = {"-xc++"}; + CDB.ExtraClangFlags = {"-xobjective-c++"}; // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the // expected rename occurrences. struct Case { @@ -1557,13 +1567,12 @@ } )cpp", }, - { - // virtual templated method - R"cpp( + {// virtual templated method + R"cpp( template class Foo { virtual void [[m]](); }; class Bar : Foo { void [[^m]]() override; }; )cpp", - R"cpp( + R"cpp( #include "foo.h" template void Foo::[[m]]() {} @@ -1571,8 +1580,7 @@ // the canonical Foo::m(). // https://github.com/clangd/clangd/issues/1325 class Baz : Foo { void m() override; }; -)cpp" - }, +)cpp"}, { // rename on constructor and destructor. R"cpp( @@ -1677,6 +1685,20 @@ } )cpp", }, + { + // Objective-C classes. + R"cpp( +@interface [[Fo^o]] +@end + )cpp", + R"cpp( +#include "foo.h" +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", + }, }; trace::TestTracer Tracer; Index: clang-tools-extra/clangd/Fin
[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
dgoldman created this revision. dgoldman added a reviewer: kadircet. Herald added a subscriber: arphaman. Herald added a project: All. dgoldman requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D152720 Files: clang-tools-extra/clangd/FindTarget.cpp clang-tools-extra/clangd/unittests/RenameTests.cpp Index: clang-tools-extra/clangd/unittests/RenameTests.cpp === --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -840,6 +840,16 @@ foo('x'); } )cpp", + + // ObjC class. + R"cpp( +@interface [[Fo^o]] +@end +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", }; llvm::StringRef NewName = "NewName"; for (llvm::StringRef T : Tests) { @@ -1468,7 +1478,7 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) { MockCompilationDatabase CDB; - CDB.ExtraClangFlags = {"-xc++"}; + CDB.ExtraClangFlags = {"-xobjective-c++"}; // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the // expected rename occurrences. struct Case { @@ -1677,6 +1687,20 @@ } )cpp", }, + { + // Objective-C classes. + R"cpp( +@interface [[Fo^o]] +@end + )cpp", + R"cpp( +#include "foo.h" +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", + }, }; trace::TestTracer Tracer; Index: clang-tools-extra/clangd/FindTarget.cpp === --- clang-tools-extra/clangd/FindTarget.cpp +++ clang-tools-extra/clangd/FindTarget.cpp @@ -707,6 +707,14 @@ /*IsDecl=*/true, {OCID->getCategoryDecl()}}); } + +void VisitObjCImplementationDecl(const ObjCImplementationDecl *OIMD) { + if (const auto *CI = OIMD->getClassInterface()) +Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(), +OIMD->getLocation(), +/*IsDecl=*/true, +{CI}}); +} }; Visitor V{Resolver}; Index: clang-tools-extra/clangd/unittests/RenameTests.cpp === --- clang-tools-extra/clangd/unittests/RenameTests.cpp +++ clang-tools-extra/clangd/unittests/RenameTests.cpp @@ -840,6 +840,16 @@ foo('x'); } )cpp", + + // ObjC class. + R"cpp( +@interface [[Fo^o]] +@end +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", }; llvm::StringRef NewName = "NewName"; for (llvm::StringRef T : Tests) { @@ -1468,7 +1478,7 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) { MockCompilationDatabase CDB; - CDB.ExtraClangFlags = {"-xc++"}; + CDB.ExtraClangFlags = {"-xobjective-c++"}; // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the // expected rename occurrences. struct Case { @@ -1677,6 +1687,20 @@ } )cpp", }, + { + // Objective-C classes. + R"cpp( +@interface [[Fo^o]] +@end + )cpp", + R"cpp( +#include "foo.h" +@implementation [[F^oo]] +@end + +void func([[Foo]] *f) {} + )cpp", + }, }; trace::TestTracer Tracer; Index: clang-tools-extra/clangd/FindTarget.cpp === --- clang-tools-extra/clangd/FindTarget.cpp +++ clang-tools-extra/clangd/FindTarget.cpp @@ -707,6 +707,14 @@ /*IsDecl=*/true, {OCID->getCategoryDecl()}}); } + +void VisitObjCImplementationDecl(const ObjCImplementationDecl *OIMD) { + if (const auto *CI = OIMD->getClassInterface()) +Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(), +OIMD->getLocation(), +/*IsDecl=*/true, +{CI}}); +} }; Visitor V{Resolver}; ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits