sammccall added a comment.
Let's chat offline about how to land this.
Main points are:
- we may just be able to use the AutoTypeLoc instead of the separate
RecursiveASTVisitor. (If so, we can do more checks in prepare())
- I have lots of questions about where to draw the line on pretty-printing, and
whether the initial version here is a useful first step. Maybe we should just
move this part to AST.h and stub it out, with a plan to incrementally improve
later.
================
Comment at: clang-tools-extra/clangd/AST.h:20
#include "clang/Lex/MacroInfo.h"
+#include "clang/AST/RecursiveASTVisitor.h"
----------------
(can now revert this change)
================
Comment at: clang-tools-extra/clangd/XRefs.h:143
+/// Retrieves the deduced type at a given location (auto, decltype).
+/// SourceLocationBeg must point to the first character of the token
----------------
Can you elaborate slightly whether this returns the AutoType/DecltypeType or
its underlying type?
================
Comment at: clang-tools-extra/clangd/XRefs.h:144
+/// Retrieves the deduced type at a given location (auto, decltype).
+/// SourceLocationBeg must point to the first character of the token
+llvm::Optional<QualType> getDeducedType(ParsedAST &AST,
----------------
nit: I'd change the second sentence to be "Retuns None unless SourceLocationBeg
starts an auto/decltype token".
The current wording isn't totally specific about the token, or what happens in
other cases (UB is common for preconditions)
================
Comment at: clang-tools-extra/clangd/XRefs.h:150
+/// SourceLocationBeg must point to the first character of the token
+bool hasDeducedType(ParsedAST &AST, SourceLocation SourceLocationBeg);
+
----------------
This appears to be unused, and isn't really worth having unless it's
significantly more efficient than getDeducedType().hasValue() and we think that
matters.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:32
+ CachedLocation = findAutoType(Node);
+ return CachedLocation != llvm::None;
+}
----------------
&& `!CachedLocation->getTypePtr()->getDeducedType()->isNull()`?
to avoid triggering in e.g. dependent code like
```
template <typename T> void f() {
auto X = T::foo();
}
```
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:39
+ llvm::Optional<clang::QualType> DeductedType =
+ getDeducedType(Inputs.AST, CachedLocation->getBeginLoc());
+
----------------
is this ever not just `CachedLocation->getTypePtr()->getDeducedType()`?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:47
+ // if it's a lambda expression, return an error message
+ if (isa<RecordType>(*DeductedType) and
+ dyn_cast<RecordType>(*DeductedType)->getDecl()->isLambda()) {
----------------
again, this is actually a cheap test (if we don't need to use the deduced type
visitor), we can lift it into prepare
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:53
+
+ SourceRange OriginalRange(CachedLocation->getBeginLoc(),
+ CachedLocation->getEndLoc());
----------------
nit: this is just CachedLocation->getSourceRange()
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:55
+ CachedLocation->getEndLoc());
+ PrintingPolicy PP(Inputs.AST.getASTContext().getPrintingPolicy());
+ PP.SuppressTagKeyword = 1;
----------------
this logic around pretty-printing a type for insertion in code is going to be:
- eventually more complicated than this: (e.g. this won't trim the "clangd"
from `std::vector<clangd::Tweak>`)
- needed in several places, like "extract expression" refactoring
- fairly easily isolated from the rest of the code here
Can we move this to AST.h, with some signature like:
`std::string printType(QualType, DeclContext &)`
Note that the namespace and (most) governing using decls/directives can be
obtained from the DC (printNamespaceScope in AST.h).
The limitations in the current version are fine for now, but we should document
them.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:68
+const llvm::Optional<AutoTypeLoc>
+ExpandAutoType::findAutoType(const SelectionTree::Node* StartNode) {
+ auto Node = StartNode;
----------------
I'm confused about this - how can the user select the child of an AutoTypeLoc
rather than the AutoTypeLoc itself? i.e. do we need the loop?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:70
+ auto Node = StartNode;
+ while (Node != nullptr) {
+ auto TypeNode = Node->ASTNode.get<TypeLoc>();
----------------
(nit: for loop in disguise!)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:108
+ const SelectionTree::Node* StartNode) {
+ auto Node = StartNode;
+ while (Node != nullptr) {
----------------
also a for loop in disguise
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:111
+ LLVM_DEBUG(Node->ASTNode.print());
+ if (const Decl* Current = Node->ASTNode.get<Decl>()) {
+ if (auto CurrentNameSpace = dyn_cast<NamespaceDecl>(Current)) {
----------------
nit: you can combine these with
`if (const auto* NSD =
dyn_cast_or_null<NamespaceDecl>(Node->ASTNode.get<Decl>()))`
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:112
+ if (const Decl* Current = Node->ASTNode.get<Decl>()) {
+ if (auto CurrentNameSpace = dyn_cast<NamespaceDecl>(Current)) {
+ return CurrentNameSpace->getQualifiedNameAsString();
----------------
nit: prefer auto*
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:113
+ if (auto CurrentNameSpace = dyn_cast<NamespaceDecl>(Current)) {
+ return CurrentNameSpace->getQualifiedNameAsString();
+ }
----------------
with namespaces, both inline and (probably) anonymous namespaces should be
ignored, which I don't think this function will do.
note there can also be non-namespace qualifiers like `MyClass::MyNestedClass`.
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:396
+ )cpp";
+ checkTransform(ID, Input, Output);
+}
----------------
can we have one for function pointers?
```
int foo();
auto x = &foo;
```
The correct replacement would be `int (*x)() = &foo;` but I think we should
just aim to produce no edits in such cases.
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:2111
+TEST(GetDeductedType, KwAutoExpansion) {
+ struct Test {
----------------
(still "deducted" here)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62855/new/
https://reviews.llvm.org/D62855
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
