dgoldman added inline comments.
================
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()) {
----------------
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?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:176
+ // it returns nil (setter method must have a void return type).
+ return "";
+ } else if (PR->isMessagingGetter()) {
----------------
sammccall wrote:
> please don't use "" as a sentinel value, it will lead to bugs
>
> Why are we checking this here rather than in eligibleForExtraction?
I put it in the ExtractionContext constructor since we would otherwise have to
perform the same logic twice to compute the type string. I could make it into a
function to be shared by both if you'd like?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:208
+ std::string ExtractedVarDecl =
+ Type + " " + VarName.str() + " = " + ExtractionCode.str() + "; ";
return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
----------------
sammccall wrote:
> Type + Varname isn't correct for types in general, particularly those that
> use declarator syntax.
> printType with varname as the placeholder should do what you want.
>
> Can you add a testcase where the variable has function pointer type?
I went ahead and added this but I couldn't think of a way to get a result like
this without enabling this for MemberExprs, so I went ahead and re-enabled for
them since I think it could be useful for them. LMK if I should revert that
change. I guess if we don't want to support MemberExprs I should disable the
ObjC property support since it probably makes to match the C++ equivalent?
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
