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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
