sammccall added inline comments.
================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:418 + if (const auto *ME = dyn_cast<MemberExpr>(E)) + if (const auto *TE = dyn_cast<CXXThisExpr>(ME->getBase())) + if (TE->isImplicit()) ---------------- dgoldman wrote: > sammccall wrote: > > oops, I forgot one detail: we want ME->getBase()->IgnoreImpCasts() > > > > (in `void nonConstMethod() { constMethod(); }` there's an ImplicitCastExpr > > in there... > Hmm, is this right, I tested out that example and it seems like that's > actually a `CXXMemberCallExpr` which this doesn't handle? The callee of the CXXMemberCallExpr is the MemberExpr i'm talking about here. ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:170 + if (E->hasPlaceholderType(BuiltinType::PseudoObject)) { + if (const auto *PR = dyn_cast<ObjCPropertyRefExpr>(E)) { + if (PR->isMessagingSetter()) { ---------------- dgoldman wrote: > dgoldman wrote: > > sammccall wrote: > > > dgoldman wrote: > > > > sammccall wrote: > > > > > E->getObjCProperty()? > > > > Hmm I'm not sure when that would be useful looking at the logic there, > > > > can you give an example just to make sure I understand what it would > > > > handle? > > > it's similar to the cast you have, but in addition to handling `foo.bar` > > > it handles parens like `(foo.bar)`, commas like `(0, foo.bar)` and casts. > > > All together I think this covers more cases where a pseudo-object can be > > > used as an lvalue and potentially modified. > > Gotcha, thanks. That makes sense, but it doesn't look like that has any > > usages and I'm not sure if it's safe to call from this context (e.g. if we > > can have a non ObjC property PseudoObject expr). I guess I could modify > > getObjCProperty to return nullptr in those invalid cases, WDYT? > @sammccall WDYT about this? Leave as is? Yeah, that function is not ideal. Leaving as-is sounds fine to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124486/new/ https://reviews.llvm.org/D124486 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits