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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks like a useful feature, I had been thinking about adding the equivalent for C++...

High level feedback:

- this should generate both the declaration and definition
- i doubt the interaction around subsetting fields is going to be satisfying
- don't spend complexity on lots of options/micromanaging formatting until basic functionality is solid



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:66
+
+// Returns the effective begin loc for D, including attached leading comments.
+static SourceLocation effectiveBeginLoc(const Decl *D,
----------------
If we're going to care about this (which makes sense), this should be a rangeIncludingComment() in AST.h or so


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:85
+    if (SM.isBeforeInTranslationUnit(DeclEnd, CommentEnd)) {
+      return CommentEnd;
+    }
----------------
the two returns are inconsistent, this one is the first position outside the range, the other return is the last token (not character) in the range


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:91
+
+struct LocationWithPadding {
+  SourceLocation Loc;
----------------
We try not to spend complexity on formatting beyond what clang-format cares about. 

If you're inserting before a member (or @end), always inserting 2 newlines seems close enough? (clang-format will strip consecutive blank lines)


================
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()) {
----------------
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?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:141
+    Name = ID.getName();
+    if (Name.startswith("_"))
+      Name = Name.substr(1);
----------------
Name.consume_front("_"); (FWIW I don't think this needs a comment)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:199
+
+  // We support the following selected decls:
+  // - ObjCInterfaceDecl/ObjCImplementationDecl only - generate for all
----------------
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...


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:218
+    } else if (const auto *ID = dyn_cast<ObjCImplementationDecl>(DC)) {
+      Container = ID;
+    }
----------------
while here, also grab the InterfaceDecl into another field so you don't have to go casehunting again in initParams?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:224
+
+void ObjCMemberwiseInitializer::initParams(
+    const SelectionTree::Node *N, SmallVectorImpl<MethodParameter> &Params) {
----------------
can you rename this method to something clearer?
unclear whether "init" stands for "initialize" or "initializer" here, which would refer to pretty different things!


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:251
+  }
+  // If just the Container itself was selected, fetch all properties and ivars
+  // for the given class.
----------------
Inferring the container was selected by looking at HadBadNode/Params seems very indirect.
Why not handle this case at the top? `if (Container == N->ASTNode.get<ObjcContainerDecl>()) return getAllParams(Iface)`



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:282
+  const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
+  if (!N || !Container)
+    return error("Invalid selection");
----------------
!Container isn't possible here


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:285
+
+  SmallVector<MethodParameter, 8> Params;
+  initParams(N, Params);
----------------
nit: why isn't this a return value?


================
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'));
----------------
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...)


================
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'));
+
----------------
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


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:295
+    if (GenerateImpl) {
+      Text.append({"- (instancetype)init {\n  self = [super init];\n  if "
+                   "(self) {\n\n  }\n  return self;\n}"});
----------------
This is hard to read, use a raw string


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:298
+    } else {
+      Text.append({"- (instancetype)init;"});
+    }
----------------
nit: why braces? (and elsewhere)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp:320
+
+  const SourceManager &SM = Inputs.AST->getSourceManager();
+
----------------
you need to verify that Loc is actually in the main file :-(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116385