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

[David Goldman via Phabricator via cfe-commits]

dgoldman marked 3 inline comments as done.
dgoldman added inline comments.


================
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) {
----------------
sammccall wrote:
> 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
I think this should be fine to start off, we probably want to be after class methods.


================
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'));
----------------
sammccall wrote:
> 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.
I changed it to add the declaration and impl if you select the implementation, could also do the same for selecting the interface if the impl is still visible, WDYT? Also had to change some editing logic, LMK what you think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116385