Page MenuHomePhabricator

[clangd][ObjC] Fix issue completing a method decl by name
ClosedPublic

Authored by dgoldman on Apr 19 2021, 2:57 PM.

Details

Summary

When completing an Objective-C method declaration by name, we need to
preserve the leading text as a qualifier so we insert it properly
before the first typed text chunk.

Diff Detail

Event Timeline

dgoldman created this revision.Apr 19 2021, 2:57 PM
dgoldman requested review of this revision.Apr 19 2021, 2:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2021, 2:57 PM
dgoldman updated this revision to Diff 338655.Apr 19 2021, 3:04 PM

Minor comment update

The test results look good, but I have trouble following the code, I think more comments might help :-)

clang-tools-extra/clangd/CodeCompletionStrings.cpp
140

can we say something more concrete than "the prefix of our snippet"? this is the -(void) part right?

151

is this TODO handled now?

155

brief comment here should explain the significance of !HadInformativeArguments (what it means if we know we're handling objc)

Maybe even worth an example here, not obvious to me what the data is that we're shuffling between variables here.

203

informative chunks? Not sure how we'd know they're arguments here.

dgoldman updated this revision to Diff 340619.Apr 26 2021, 1:10 PM
dgoldman marked 4 inline comments as done.

Address review comments

clang-tools-extra/clangd/CodeCompletionStrings.cpp
151

No, this does not change any behavior for parameters (we don't show the full ObjcMethodDecl signature including previous parameter types if you complete it from the middle)

Friendly ping

sammccall accepted this revision.Tue, Jun 1, 8:51 AM

Thanks for the detailed comments, this makes a lot more sense to me now!

clang-tools-extra/clangd/CodeCompletionStrings.cpp
160

Does completing the no-args declaration - (void) foo work as expected?

We never get to this part of the code in this case because endswith(":") is never true. The comment above says "safe to treat as c++" but not sure this is true for declaration, just method calls. Maybe it works for some other reason though?

In any case, it's OK if this case doesn't work (this patch still improves things a lot). We should probably have a test showing what the state is.

This revision is now accepted and ready to land.Tue, Jun 1, 8:51 AM
dgoldman updated this revision to Diff 349014.Tue, Jun 1, 10:27 AM

Add another test for a simple method decl (no arg selector)

dgoldman marked an inline comment as done.Tue, Jun 1, 10:31 AM
dgoldman added inline comments.
clang-tools-extra/clangd/CodeCompletionStrings.cpp
160

Yeah, it does. I added a test case for it, it does work, the current behavior is fine, the issue for ObjC is specifically selectors with arguments.

This revision was landed with ongoing or failed builds.Tue, Jun 1, 10:44 AM
This revision was automatically updated to reflect the committed changes.
dgoldman marked an inline comment as done.