Page MenuHomePhabricator

[clangd] Implement semantic token modifier "definition"
Needs ReviewPublic

Authored by ckandeler on Jun 9 2022, 7:38 AM.

Diff Detail

Event Timeline

ckandeler created this revision.Jun 9 2022, 7:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 7:38 AM
ckandeler requested review of this revision.Jun 9 2022, 7:38 AM
ckandeler updated this revision to Diff 435547.Jun 9 2022, 7:40 AM

Indentation.

ckandeler updated this revision to Diff 435561.Jun 9 2022, 8:24 AM

Addressed clang-format complaints.

Trass3r added a subscriber: Trass3r.Jun 9 2022, 9:26 AM
dgoldman added inline comments.
clang-tools-extra/clangd/SemanticHighlighting.cpp
873–875

Do we need similar code for ObjCProtocolDecl? Also should ObjCImplDecl be considered definitions here?

nridge added a subscriber: nridge.Jun 12 2022, 12:06 AM
ckandeler added inline comments.Jun 12 2022, 11:53 PM
clang-tools-extra/clangd/SemanticHighlighting.cpp
873–875

Possibly. I know next to nothing about Objective C, so I'll just do as I'm told here. On a related note, the code above regarding ObjCMethodDecl does not seem to do anything, i.e. none of the constructs that to my eye appear to be Objective-C methods get the "def" modifier.

dgoldman added inline comments.Jun 13 2022, 8:20 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
873–875

Gotcha, yeah I think it makes sense to do the same for ObjCImplDecl, ObjCProtocolDecl, and ObjCCategoryDecl.

re: methods, ah yeah, that's because canHighlightName will return false since we need to special case handling for ObjC methods since their names can be split across non contiguous tokens (selectors). Instead, would need to update VisitObjCMethodDecl and highlightObjCSelector.

Objective-C improvements.

ckandeler updated this revision to Diff 448655.Jul 29 2022, 9:11 AM

Addressed remaining Objective-C issues

dgoldman added inline comments.Thu, Sep 8, 11:58 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
863

Instead of doing it like this, I wonder if would make more sense to call getDefinition from https://cs.github.com/llvm/llvm-project/blob/ae071a59bc6cc09e0e0043e0ef1d9725853f1681/clang-tools-extra/clangd/XRefs.cpp#L76 and if it matches Decl, add the Definition modifier?

ckandeler added inline comments.Mon, Sep 12, 2:14 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
863

Won't that incur an additional look-up or some other type of work? I'm not deeply familiar with the implementation, but a cursory glance seems to suggests that isThisDeclarationADefinition() is just an accessor, while getDefinition() "does things".