This is an archive of the discontinued LLVM Phabricator instance.

[CodeCompletion] Add block placeholders when completing member access for Objective-C block property setters
ClosedPublic

Authored by arphaman on Oct 12 2016, 9:10 AM.

Details

Summary

This patch depends on https://reviews.llvm.org/D25519.

This patch changes the way that Objective-C block properties are code complete: clang now suggests completion with an additional '=' and the block literal placeholder when providing member access completions for appropriate readwrite block properties. This patch uses a simple heuristic to determine if it's appropriate to suggest a setter completion for block properties: if we are completing member access that is a standalone statement, we provide setter completion. Otherwise we fallback to the default property completion.

The following example illustrates when the setter completion is triggered:

@interface Test : Obj
@property (readonly, nonatomic, copy) void (^onReadonly)(int *someParameter);
@end
@implementation Test
- foo {
  self.<COMPLETION> // will complete with ‘=‘ and block
}
- fooNot {
  // These will code complete normally:
  (self.<COMPLETION>)
  return self.<COMPLETION>
  [self foo: self.<COMPLETION>]
  if (self.<COMPLETION>) { }
}
@end

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 74390.Oct 12 2016, 9:10 AM
arphaman retitled this revision from to [CodeCompletion] Add block placeholders when completing member access for Objective-C block property setters.
arphaman updated this object.
arphaman added reviewers: manmanren, doug.gregor.
arphaman set the repository for this revision to rL LLVM.
arphaman added a subscriber: cfe-commits.
akyrtzi added a subscriber: akyrtzi.EditedOct 12 2016, 11:27 AM

What if the user just wants to invoke the block, this is as common or more, like:

self.onEventHandler(10)

The assign literal completion is useful but it should be an additional entry (with maybe lower priority) not replace the property completion.
BTW, it would be great if we had completion as if it was a function call, so 2 entries, 1 for block invocation and 1 for assigning literal. Absence the block invocation call it should be 1 for normal property completion and 1 for assigning literal.

What if the user just wants to invoke the block, this is as common or more, like:

self.onEventHandler(10)

The assign literal completion is useful but it should be an additional entry (with maybe lower priority) not replace the property completion.
BTW, it would be great if we had completion as if it was a function call, so 2 entries, 1 for block invocation and 1 for assigning literal. Absence the block invocation call it should be 1 for normal property completion and 1 for assigning literal.

You're right, I forgot about the most common use of block properties. I will update the patch today.

It makes sense to complete block invocation as well, but that has to be done in a separate patch. I suppose that completion for block invocations should be provided for normal sub expressions as well then.

arphaman updated this revision to Diff 74528.Oct 13 2016, 8:36 AM

The updated patch now treats the block property setter completion as a completion result that's additional to the default property completion result.

Right now I gave the setter completion a flat priority bump of 3. Should something different be used? What do you think of the following possible priority heuristic: when completing blocks properties that return void the default property completion result should show up before the setter, otherwise the setter should show up before the property (we normally don't want to ignore the result of the call, right)?

What do you think of the following possible priority heuristic

SGTM.

Changes LGTM. I'd also recommend that as a follow-up patch it would be great to extend the setter completion to variables as well (global variables, fields, ivars, etc.)

Another recommendation for follow-up. When invoking completion on the right-hand side of the assignment it should provide a block literal completion with high priority. For example, when completing like this:

self.foo = <COMPLETE>

What do you think of the following possible priority heuristic

SGTM.

Changes LGTM. I'd also recommend that as a follow-up patch it would be great to extend the setter completion to variables as well (global variables, fields, ivars, etc.)

Thanks, I will commit this as it is. I will post a follow-up patch that uses the priority heuristic that I described above, and will work on related patches that you mentioned as well.

This revision was automatically updated to reflect the committed changes.