sammccall added a comment.
Sorry about the delay here. Happy to chat offline if I'm being confusing.
================
Comment at: clang-tools-extra/clangd/AST.cpp:172
+namespace {
+/// Computes the deduced type at a given location by visiting the relevant
----------------
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?
================
Comment at: clang-tools-extra/clangd/AST.h:72
+// TODO: add documentation
+llvm::Optional<QualType> getDeductedType(SourceLocation SearchedLocation,
ASTContext &AST);
+
----------------
nit: deduced
(also docs! In particular the fact that the SearchedLocation should point to
exactly an `auto`, `decltype` etc token)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:76
+std::string
+ExpandAutoType::shortenNamespace(const llvm::StringRef &OriginalName,
+ const llvm::StringRef &CurrentNamespace) {
----------------
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.
================
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:
> 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.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.h:1
+//===--- Tweak.h -------------------------------------------------*-
C++-*-===//
+//
----------------
update filename
(these headers are so silly :-()
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.h:21
+/// std::string x = Something();
+/// ^^^^^^^^^^^
+class ExpandAutoType : public Tweak {
----------------
Maybe add a FIXME to handle decltype here too? It's pretty similar (though
rarer).
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:221
+TEST(TweakTest, ExpandAutoType) {
+ llvm::StringLiteral ID = "ExpandAutoType";
----------------
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)
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
