[PATCH] D124486: [clangd] ExtractVariable support for C and Objective-C

[Sam McCall via Phabricator via cfe-commits]

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