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
