sammccall added a comment.
LG apart from a few nits & reverting the whitespace changes to unrelated tests.
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:435
void VisitObjCObjectType(const ObjCObjectType *OOT) {
- // FIXME: ObjCObjectTypeLoc has no children for the protocol list, so
- // there is no node in id<Foo> that refers to ObjCProtocolDecl Foo.
- if (OOT->isObjCQualifiedId() && OOT->getNumProtocols() == 1)
- Outer.add(OOT->getProtocol(0), Flags);
+ unsigned NumProtocols = OOT->getNumProtocols();
+ for (unsigned I = 0; I < NumProtocols; I++)
----------------
dgoldman wrote:
> Let me know if I'm missing something here, not exactly sure what the old
> comment meant - is there a problem since this ObjCObjectType can refer to
> multiple protocols?
Here's the way the AST should ideally look like (and what this comment is
wishing for):
```
ObjCObjectTypeLoc C<Foo>
- ObjCInterfaceTypeLoc C
- ObjCProtocolLoc Foo // doesn't actually exist
```
And targetDecl() would work on the two children, but not the parent.
But there's no AST node for protocols. A piece of cleverness in this patch is
getting go-to-definition to work on protocols by saying "if the cursor is on
ObjCObjectType itself, then it must be on a protocol rather than the base
(otherwise it'd be on the ObjCInterfaceType child)".
But it's a hack, as evidenced by the fact that operations that work on the
*range* `[[C<Foo>]]` will still see the target as `<Foo>` (literally any of
nothing, C, C+Foo would be better).
Nevertheless I think it's probably a net win.
---
Can you leave a comment here like
```
// There's no child node for just a protocol, so make all the protocols targets.
// But not the base type, which *does* have a child ObjCInterfaceTypeLoc.
// This structure is a hack, but it works well for go-to-definition.
```
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:824
return;
+ ReferenceLoc *Ref = &Refs.back();
+ // Fill in the qualifier.
----------------
this doesn't seem right: we now have a list, the Visit() call may have added
things to the list, we need to adjust *all* the added things.
```
// Add qualifier for the newly-added refs.
for (unsigned I = InitialSize; I < Refs.size(); ++I) {
// adjust Refs[I]
}
```
================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:912
+ }
+ // FIXME: Maybe add references to protocols in ObjCObjectPointerTypeLoc.
+ }
----------------
dgoldman wrote:
> Doesn't look like that's needed, but if we wanted to decorate the type
> parameters in `ObjCObjectTypeLoc` with `typeParameter` would that just go in
> `SemanticHighlighting.cpp`?
That's not something we want to do - ObjCObjectTypeLoc has type arguments, not
parameters.
(Not trying to be pedantic with terminology, but the difference is critical
here)
e.g. in the function call `foo(42)` we don't decorate 42 as a parameter, we
decorate it as an integer.
================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:952
)cpp";
- // FIXME: there's no AST node corresponding to 'Foo', so we're stuck.
- EXPECT_DECLS("ObjCObjectTypeLoc");
+ EXPECT_DECLS("ObjCObjectTypeLoc", "@protocol Foo");
}
----------------
can you add a second protocol so we can see the boundaries of this approach?
(I think C<[[Foo]], Bar> should resolve to both @protocol Foo and @protocol Bar)
================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:1048
)cpp",
- "0: targets = {global}\n"
- "1: targets = {param}\n"
----------------
there are a ton of whitespace changes in this patch, because the version at
HEAD isn't clang-format clean :-(
Feel free to format it first in a separate commit, but I'd prefer not to
reformat all the tests mixed in with other changes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98984/new/
https://reviews.llvm.org/D98984
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
