[PATCH] D116385: [clangd] Code action for creating an ObjC initializer

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Alright, the implementation looks good.

I think just a question of what to do when firing on @implementation. I'd suggest one of:

- generating the interface too, you have almost allthe code
- not supporting @implementation yet

We can also keep the current behavior if you feel strongly about it, but I guess it will confuse people.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:265
+  // Insert before the first non-init instance method.
+  std::vector<Anchor> Anchors = {
+      {[](const Decl *D) {
----------------
If there are no non-init instance methods this will insert at the very bottom of the `@implementation`. You can add a second anchor to cover this case.

A policy that might make sense here is: 
  {{IsInitMethod, Below}, {Anything, Above}}
But up to you


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:285
+
+  SmallVector<MethodParameter, 8> Params;
+  initParams(N, Params);
----------------
dgoldman wrote:
> sammccall wrote:
> > nit: why isn't this a return value?
> IIUC this (having the impl vector be an out param) seems to be the convention - `SmallVectorImpl(const SmallVectorImpl &) = delete;`, but I can change the return type to instead be SmallVector<MethodParameter, 8>, WDYT?
Please do.

There are a bunch of reasons that vector out-params are sometimes used in LLVM:
 - caller rather than callee should choose the SmallVector size
 - repeated calls to append to an existing vector
 - return value is something else
 - some old pre-C++11 code

but I think none apply here


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:290
+
+  bool GenerateImpl = isa<ObjCImplementationDecl>(Container);
+  llvm::SmallString<256> Text(std::string(LocPadding.PadStart, '\n'));
----------------
dgoldman wrote:
> sammccall wrote:
> > Wait, so if the user triggers the action on @interface then they'll get a decl with no impl, and if they trigger on @implementation they'll get the opposite?
> > 
> > Seems like we should at least try to generate both, no? (May not be possible if we're triggering on @interface and index lookup fails, but still...)
> Yeah, I decided to avoid this due to the complexity for a v1, could look into adding it in a follow up. For sourcekit-lsp, it would only work if you've opened the impl first (since it will only have the dynamic index available). Is there anything I could look at which does something similar?
This seems like it's going to be a surprising limitation. You can mark it the tweak as "hidden" if you don't want to expose it to users yet.

Is there much complexity in generating both if targeting the @implementation? It seems you've already got both codepaths and just need to run both. If that's too complicated, it doesn't really seem like a good idea to support this on implementations, generating a definition for a member that doesn't exist is confusing.

(I'd suggest changing the description to "declare" memberwise initializer whenever you're not generating the definition.)

> For sourcekit-lsp, it would only work if you've opened the impl first

The general approach is to find the target file and then pseudoparse the bits you need, as you won't have an AST in any case. So it will still work without index if the file can be found using filename heuristics.

> Is there anything I could look at which does something similar?

DefineOutline does something similar: it triggers on a declaration and edits the declaration and inserts a definition.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116385/new/

https://reviews.llvm.org/D116385