sammccall added inline comments.
================ Comment at: clang-tools-extra/clangd/FindTarget.cpp:360 + dyn_cast<ObjCImplementationDecl>(D)) { + // Objective-C implementation should map back to its interface. + D = IID->getClassInterface(); ---------------- This just describes what the code is doing, say why instead? // We treat ObjC{Interface,Implementation}Decl as if they were a decl/def pair. ================ Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:696 + )cpp"; + EXPECT_DECLS("ObjCImplementationDecl", "@interface Implicit"); + ---------------- Hmm, do we want to use the @interface or @implementation for this case? The interface is implicit but probably still has a valid location. Currently symbolcollector and findtarget do different things... ================ Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:614 + +TEST_F(SymbolCollectorTest, ObjCClassExtensions) { + Annotations Header(R"( ---------------- dgoldman wrote: > Here's the ClassExtension that I was talking about. > > Ideally we can map each > > `Cat ()` --> `@implementation Cat` like I did in XRefs > > But as you said the `Cat ()` could be in a different file and I think it has > a different USR. > > See also > https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/CustomizingExistingClasses/CustomizingExistingClasses.html#//apple_ref/doc/uid/TP40011210-CH6-SW3 > Ideally we can map each > Cat () --> @implementation Cat like I did in XRefs I'm not sure there's anything that would ideally be done differently here. The logic in xrefs is a special "go-to-definition" action - there's some ambiguity about what's being *targeted* by the user. But here there's no targeting going on, and there's no ambiguity about what's being *declared*. The thing to test would be that we're emitting *refs* from `@interface [[Cat]] ()` to catdecl. ================ Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:710 + R"objc( + @class Foo; + @interface $decl[[Foo]] ---------------- maybe add a comment like // prefer interface definition over forward declaration ================ Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:803 + }; + for (const char *Test : Tests) { + Annotations T(Test); ---------------- this seems to be copy/pasted from the test above. Is there a reason this can't be part of the test above? ================ Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:838 + +TEST(LocateSymbol, MultipleDeclsWithSameDefinition) { + // Ranges in tests: ---------------- and again here Desire to split these tables up into named tests is something we want to address somehow, but we don't have a good answer right now and it's important for maintenance that the logic/annotation conventions don't diverge across different tests that could be the same. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83501/new/ https://reviews.llvm.org/D83501 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits