sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Sorry *again* about the delayed response, will explain off-list.
This looks great. There's a substantive question about `auto &x⟦: int &⟧ = 42`
vs `auto⟦int⟧ &x = 42`. But I don't think this should block landing this unless
you want to.
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:33
+ TypeHintPolicy.SuppressScope = true; // keep type names short
+ TypeHintPolicy.AnonymousTagLocations =
+ false; // do not print lambda locations
----------------
For lambda values, the overwhelmingly common case is `auto X = <lambda-expr>`.
A few reasons we may want to not show these at all (and so omit type hints for
lambdas as a first approximation):
- the fact that it's a lambda is pretty obvious
- lambdas very often have complex introducers (captures/params) that a hint
will displace/add noise to
- we may want to add type hints to captures-with-initializers, which are very
much like auto-typed variables, and the fewer hints we have to cram on a line
the better
(fine to consider this stuff later, just wanted to mention it)
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:76
+ bool VisitVarDecl(VarDecl *D) {
+ // Structured binding - not handled yet.
+ if (isa<DecompositionDecl>(D))
----------------
FWIW I think hinting the individual bindings will be more useful and also more
consistent with the other hints you have here.
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:81
+ if (auto *AT = D->getType()->getContainedAutoType()) {
+ if (!D->getType()->isDependentType()) {
+ addInlayHint(D->getLocation(), InlayHintKind::TypeHint,
----------------
why this check vs checking whether AT is deduced?
(thus fixing the dependent `T` testcase)
At first I assumed this was the usual "AutoType in AST doesn't store the
deduced type" problem, but shouldn't it work properly if we're starting at the
variable type?
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:82
+ if (!D->getType()->isDependentType()) {
+ addInlayHint(D->getLocation(), InlayHintKind::TypeHint,
+ ": " + D->getType().getAsString(TypeHintPolicy));
----------------
This places the hint on the variable rather than the `auto`. And fittingly (but
subtly), it prints type rather than just the placeholder type (e.g. `const
auto& x = 42` => `const int&`, not `int`)
This decision is worth a comment, and probably a little discussion :-)
The alternative of annotating `auto` has advantages:
- the deduced `auto` type is the most obvious question when reading the code
- that `auto` deduces to a particular type is the "correct" model, and may
lead to better language understanding. Extreme unrealistic example: in `auto x
= foo(), *y = bar();` I'd strongly prefer one hint.
- the hint will often be shorter, which is an important consideration since
we're reflowing column-limited text
- the annotated code more closely resembles code that does not use `auto`
And annotating the variable has advantages too:
- the type of the variable is the most directly important fact
- generalizes better to cases where `auto` is not present (lambda captures,
individual structured bindings)
- i suspect simpler implementation
My intuition is to expect annotating `auto` to be a little nicer, mostly
because it will come closer to making code read the same way whether it uses
`auto` or not. I'm not certain though. Interested if you have strong feelings
about this!
To save a round-trip: a likely conclusion is "maybe annotating auto is better,
but it's not clear and/or it's a bunch of work". That's 100% fine, but we
should just leave a comment whether this is something decided against vs
desirable but deferred vs unsure.
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:445
+ assertTypeHints(R"cpp(
+ auto $L[[L]] = [](int a, int b) { return a + b; };
+ )cpp",
----------------
consider `= [b(1+1)](int a) { ... }` and we can consider adding type hints to b
later
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:487
+}
+
+// Annoyances:
----------------
It'd be useful to have cases showing how we handle "bulky" typenames, e.g.
`ns::OuterClass::Template<int>` which I think we render as `Template<int>`.
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:489
+// Annoyances:
+// - auto x = dyn_cast<LongTypeName>(y);
+// - stdlib algos return unwieldy __normal_iterator<X*, ...> type
----------------
not sure if you meant dynamic_cast or dyn_cast here, both are important but
distinct!
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:489
+// Annoyances:
+// - auto x = dyn_cast<LongTypeName>(y);
+// - stdlib algos return unwieldy __normal_iterator<X*, ...> type
----------------
sammccall wrote:
> not sure if you meant dynamic_cast or dyn_cast here, both are important but
> distinct!
maybe mention the other low-hanging fruit cases from the patch description too
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:490
+// - auto x = dyn_cast<LongTypeName>(y);
+// - stdlib algos return unwieldy __normal_iterator<X*, ...> type
+
----------------
The use of a `__reserved` name might be a good hint this is a structural type
whose name is not a meaningful hint. (I guess maybe that's already what you
meant!)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102148/new/
https://reviews.llvm.org/D102148
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
