sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:165
+std::string
+ExtractionContext::computeExprType(const clang::Expr *E) const {
+ if (Ctx.getLangOpts().CPlusPlus)
----------------
(does this need to be a method?)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:166
+ExtractionContext::computeExprType(const clang::Expr *E) const {
+ if (Ctx.getLangOpts().CPlusPlus)
+ return "auto";
----------------
CPlusPlus11
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:169
+ QualType ExprType = E->getType();
+ if (E->hasPlaceholderType(BuiltinType::PseudoObject)) {
+ if (const auto *PR = dyn_cast<ObjCPropertyRefExpr>(E)) {
----------------
if it has pseudo-object type and *isn't* handled by the ObjCPropertyRef case,
we should disallow extraction right?
================
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()) {
----------------
E->getObjCProperty()?
================
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()) {
----------------
please don't use "" as a sentinel value, it will lead to bugs
Why are we checking this here rather than in eligibleForExtraction?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:187
+ AttributedType::stripOuterNullability(ExprType);
+ return ExprType.getAsString();
+}
----------------
prefer printType from AST.h, which will get qualifiers, anonymous namespaces
etc right
================
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);
----------------
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?
================
Comment at:
clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp:324
+ // We don't preserve types through typedefs.
+ // FIXME: Ideally we could preserve the non-canonical type.
+ {R"cpp(typedef long NSInteger;
----------------
I'd drop the FIXME here: the fully-sugared type type of NSInteger * long is
long, not NSInteger.
(Unless you're suggesting changing this in clang)
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
