kuhnel added inline comments.
================
Comment at: clang-tools-extra/clangd/AST.cpp:172
+namespace {
+/// Computes the deduced type at a given location by visiting the relevant
----------------
sammccall wrote:
> It looks like this has been moved from somewhere (at least code looks
> familiar) but isn't deleted anywhere. (The code in XRefs is touched but
> doesn't seem to use this). Is there a reason we can't reuse one copy?
>
>
Ah interesting. It got messed up during rebase and someone implemented a
similar function in XRefs.cpp in the mean time. So I'll just move over to their
implementation. And remove the changes to AST...
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:32
+ CachedLocation = findAutoType(Node);
+ return CachedLocation != llvm::None;
+}
----------------
sammccall wrote:
> && `!CachedLocation->getTypePtr()->getDeducedType()->isNull()`?
>
> to avoid triggering in e.g. dependent code like
> ```
> template <typename T> void f() {
> auto X = T::foo();
> }
> ```
I added the new testcase and if fails with and without the added condition.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:39
+ llvm::Optional<clang::QualType> DeductedType =
+ getDeducedType(Inputs.AST, CachedLocation->getBeginLoc());
+
----------------
sammccall wrote:
> is this ever not just `CachedLocation->getTypePtr()->getDeducedType()`?
There is some difference, because quite a few tests fail, wenn I use your call.
--> Staying with current implementation
================
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()) {
----------------
sammccall wrote:
> again, this is actually a cheap test (if we don't need to use the deduced
> type visitor), we can lift it into prepare
Then I would have to move the call to `getDeducedType(...)` also to
`prepare`....
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:53
+
+ SourceRange OriginalRange(CachedLocation->getBeginLoc(),
+ CachedLocation->getEndLoc());
----------------
sammccall wrote:
> nit: this is just CachedLocation->getSourceRange()
correct.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:68
+const llvm::Optional<AutoTypeLoc>
+ExpandAutoType::findAutoType(const SelectionTree::Node* StartNode) {
+ auto Node = StartNode;
----------------
sammccall wrote:
> 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?
At first I understood that we would have to walk the AST, but it seems we don't
have to...
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:76
+std::string
+ExpandAutoType::shortenNamespace(const llvm::StringRef &OriginalName,
+ const llvm::StringRef &CurrentNamespace) {
----------------
sammccall wrote:
> sammccall wrote:
> > This is nice. We might want to move it to SourceCode.h, somewhere near
> > SplitQualifiedName. Then we'd generally unit test all the cases in
> > SourceCodeTests, and we only have to smoke test the shortening here.
> >
> > One limitation is that it only handles shortening the qualifier on the name
> > itself, but not other parts of a printed type. e.g. `namespace ns { struct
> > S(); auto* x = new std::vector<S>(); }` will expand to
> > `std::vector<ns::S>`, not `std::vector<S>`. To fully solve this we may want
> > to modify PrintingPolicy at some point, though we could probably get a long
> > way by searching for chunks of text inside printed types that look like
> > qualified names.
> >
> > I think it might more clearly communicate the limitations if this function
> > operated on the scope part only, e.g. `printScope("clang::clangd::",
> > "clang::") --> "clangd::"`.
> >
> > In the general case, `CurrentNamespace` should be a vector, because there
> > can be others (introduced by using-declarations). Fine to leave this as a
> > fixme if it's hard to do here, though.
> StringRefs are passed by value. They're just shorthand for a char* + length.
1) added documentation on that.
2) In general it would be nicer to do these operations on a some model and not
on bare strings. We could probably recursively search for more namespaces
inside angle brackets <>...
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:221
+TEST(TweakTest, ExpandAutoType) {
+ llvm::StringLiteral ID = "ExpandAutoType";
----------------
sammccall wrote:
> can you add testcases for:
> - unknown types in a template `template <typename T> void x() { auto y =
> T::z(); }`
> - broken code `auto x = doesnt_exist();`
> - lambda `auto x = []{};`
> - inline/anon namespace: `inline namespace x { namespace { struct S; } }
> auto y = S();` should insert "S" not "x::S" or "(anonymous)::S".
> - local class: `namespace x { void y() { struct S{}; auto z = S(); } }`
> (should insert "S")
>
> (it's ok if the behavior is bad in these cases, we can fix that later. Ideal
> in first 3 is no change)
Did not add tests, as the code does not yet support this:
* `inline/anon namespace: inline namespace x { namespace { struct S; } } auto y
= S();` should insert "S" not "x::S" or "(anonymous)::S".
* `local class: namespace x { void y() { struct S{}; auto z = S(); } }` (should
insert "S")
This requires some changes on how I identify the namespaces. I would do that in
a second version/increment of the feature.
Working:
* unknown types in a template template <typename T> void x() { auto y = T::z();
}
* broken code auto x = doesnt_exist();
* lambda auto x = []{};
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:396
+ )cpp";
+ checkTransform(ID, Input, Output);
+}
----------------
sammccall wrote:
> 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.
You're an infinite oracle of strange C++ corner cases ;)
I added a test and another check for this case...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62855/new/
https://reviews.llvm.org/D62855
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
