[PATCH] D62855: [clangd] Implementation of auto type expansion.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Great, let's land this!
We can enhance further later, but this covers the most common cases.

(I think the remaining comments are trivial - then I/you can land without further review)



================
Comment at: clang-tools-extra/clangd/AST.cpp:181
+
+  u_int DifferentAt = 0;
+  while (DifferentAt < MinLength &&
----------------
unsigned rather than u_int (just common llvm style)


================
Comment at: clang-tools-extra/clangd/AST.h:79
+/// Example: shortenNamespace("ns1::MyClass<ns1::OtherClass>", "ns1")
+///    --> "MyClass<ns1::OtherClass>"
+std::string  shortenNamespace(const llvm::StringRef OriginalName,
----------------
can you leave a FIXME that this should take a DeclContext for lookup instead of the current namespace name, take in to account using directives etc?
(Otherwise it's not obvious why it belongs in `AST` instead of `SourceCode` - but it does)


================
Comment at: clang-tools-extra/clangd/Selection.cpp:369
 
+const DeclContext* SelectionTree::getDeclContext(const Node* StartNode) {
+
----------------
I think this function is not quite correct. At least, it returns the enclosing DC outside the enclosing Decl, but that doesn't seem quite right.

There are three critical cases:
 - StartNode is a DeclContext
 - StartNode is (e.g.) an Expr whose containing Decl is a DeclContext
 - StartNode is an Expr whose containing Decl is **not** a DeclContext

Currently, you return the second, second, and first DeclContext encountered, respectively.
- If the definition is "enclosing DC that is not the node itself", it should be second, first, first.
- If the definition is "enclosing DC that may be the node itself", it should be first, first, first.

I think "enclosing DC that's not the node itself" is simplest in which case you should add something like `if (CurrentNode != StartNode) if (auto *DC = dyn_cast<DeclContext>(DC)) return DC;` in the inner if.


================
Comment at: clang-tools-extra/clangd/Selection.h:104
   const Node *root() const { return Root; }
+  // Get the DeclContext for the Node.
+  // Return nullptr if no DeclContext is found.
----------------
, which is not the node itself. (Is this the intended behavior? worth calling out either way)


================
Comment at: clang-tools-extra/clangd/Selection.h:105
+  // Get the DeclContext for the Node.
+  // Return nullptr if no DeclContext is found.
+  static const DeclContext *getDeclContext(const Node* CurrentNode);
----------------
Thinking about this again - this can't ever happen, it violates the invariant that a tree must be rooted at TranslationUnitDecl. Just return a reference and add an assert to the implementation.


================
Comment at: clang-tools-extra/clangd/Selection.h:106
+  // Return nullptr if no DeclContext is found.
+  static const DeclContext *getDeclContext(const Node* CurrentNode);
 
----------------
nit: I think this would be slightly clearer as a (non-static) member function on Node. (It also makes it obvious node can't be null).


================
Comment at: clang-tools-extra/clangd/Selection.h:106
+  // Return nullptr if no DeclContext is found.
+  static const DeclContext *getDeclContext(const Node* CurrentNode);
 
----------------
sammccall wrote:
> nit: I think this would be slightly clearer as a (non-static) member function on Node. (It also makes it obvious node can't be null).
please run git clang-format.
(LLVM uses `type *var` not `type* var`)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:88
+
+  // if it's a function expression, return an error message
+  if (isa<PointerType>(*DeducedType) and
----------------
comment should say why
`// naively replacing 'auto' with the type will break declarations.`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:89
+  // if it's a function expression, return an error message
+  if (isa<PointerType>(*DeducedType) and
+      dyn_cast<PointerType>(*DeducedType)->isFunctionPointerType()) {
----------------
no need for this first check/cast: isFunctionPointerType is a member of Type


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:90
+  if (isa<PointerType>(*DeducedType) and
+      dyn_cast<PointerType>(*DeducedType)->isFunctionPointerType()) {
+    return createErrorMessage("Could not expand type of function pointer",
----------------
please also check isFunctionReferenceType, isMemberPointerType, isArrayType for the same reason


================
Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:34
+
+  /* FIXME: template types are ignored right now, we should add this in the
+   * future
----------------
rather than commenting out the test, assert the (bad) behavior and keep the comment.
That way someone gets a happy surprise when they fix it :-)


================
Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:42
+
+//FIXME: add tests for printType
+
----------------
delete the comment unless the tests will be added in this patch :-)


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:23
 #include <cassert>
+//#include "refactor/tweaks/ExpandAutoType.cpp"
 
----------------
remove


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62855/new/

https://reviews.llvm.org/D62855