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