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

[David Goldman via Phabricator via cfe-commits]

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


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:113
+    if (MD->getMethodFamily() == OMF_init) {
+      Loc = effectiveEndLoc(MD, SM).getLocWithOffset(1);
+    } else if (Loc.isValid()) {
----------------
sammccall wrote:
> This algorithm doesn't make much sense to me. 
> You want to insert before the first non-init instance method, falling back to the end.
> So why do you need to do more than search for the first non-init instance method, and find the location before it? In particular why are we looking at init methods and locations after them?
Yeah, I think that makes more sense, changed.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:199
+
+  // We support the following selected decls:
+  // - ObjCInterfaceDecl/ObjCImplementationDecl only - generate for all
----------------
sammccall wrote:
> I understand the desire to select the particular fields, but given the limitation of the interaction I wonder whether these cases are useful & discoverable enough to be worth the complexity.
> 
> - how often is an initializer that sets one variable of many useful?
> - out of the times when a subset of variables should be initialized, how often are they contiguous in the code?
> 
> I think it would likely be better to start with initializing all members...
I didn't realize that VS Code only lets you select a range, not selecting multiple points (e.g. multiple cursors). I still think this could be useful and it isn't that hard to support, although yes, ideally we could do something like IntelliJ where it presents you a UI to select fields. The main use case of this atm is for simple model objects, and for those I think it could be useful to make some params optional although the contiguous limitation is definitely restrictive.

FWIW I don't think code actions in general are particularly discoverable.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:285
+
+  SmallVector<MethodParameter, 8> Params;
+  initParams(N, Params);
----------------
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?


================
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:
> 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?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:291
+  bool GenerateImpl = isa<ObjCImplementationDecl>(Container);
+  llvm::SmallString<256> Text(std::string(LocPadding.PadStart, '\n'));
+
----------------
sammccall wrote:
> For building strings like this with formatting, we usually use raw_string_ostream and `formatv`.
> This would make the stuff starting at 303 easier to read
Yep that helps quite a bit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116385