[PATCH] D151785: [clangd] Desugar dependent type aliases for auto type hints
Nathan Ridge via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 5 00:17:12 PDT 2023
nridge added a comment.
Thanks, this is a nice improvement.
A couple of general comments:
- We have an existing heuristic here <https://searchfox.org/llvm/rev/3a458256ee22a0e7c31529de42fa6caa263d88fe/clang-tools-extra/clangd/InlayHints.cpp#667-677>, should we move the logic into `maybeDesugar()` so it's all in one place?
- I don't think "dependent type" is the right terminology, "dependent type" means "depends on a template parameter //whose value is not yet known//", for example `vector<T>::value_type` where we don't know the value of `T`. But here we are talking about things like `vector<int>::value_type`. Maybe in place of "dependent type alias" we can say "alias for template parameter"?
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:198
+bool isDependentOrTrivialSugaredType(QualType QT) {
+ static auto PeelQualifier = [](QualType QT) {
+ // Neither `PointerType` nor `ReferenceType` is considered as sugared
----------------
nit: it sounds like this name (`PeelQualifier`) is using "qualifier" to mean "pointer or reference", which is different from the meaning of the word in "QualType" (where it means "const or volatile")
maybe `PeelWrappers` would be a better name, since we can think of `PointerType` and `ReferenceType` as wrapping an underlying type?
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:201
+ // type. Peel it.
+ QualType Last = QT;
+ for (QT = QT->getPointeeType(); !QT.isNull();
----------------
a possible reformulation:
```
QualType Next;
while (!(Next = QT->getPointeeType()).isNull()) {
QT = Next;
}
return QT;
```
this seems slightly cleaner to me, but I'll leave it up to you
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:207
+ };
+ for (QualType Desugared =
+ PeelQualifier(QT->getLocallyUnqualifiedSingleStepDesugaredType());
----------------
this loop can also be reformulated to avoid repeating the update step:
```
while (true) {
QualType Desugared = Peel(...);
if (Desugared == QT)
break;
if (Desugared->getAs<...>)
return true;
QT = Desugared;
}
```
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:215
+ return true;
+ if (Desugared->getAs<BuiltinType>())
+ return true;
----------------
The only test case that seems to rely on this `BuiltinType` heuristic is getting `int` instead of `size_type` for `array.size()`.
`size_type` doesn't seem so bad, maybe we can leave this out for now? Or do you have a more compelling motivating case for it?
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:235
+// parameter QT.
+QualType tryDesugar(ASTContext &AST, QualType QT) {
+ if (isDependentOrTrivialSugaredType(QT))
----------------
name suggestion: `maybeDesugar`
(`try` sounds like it might fail and return something that needs to be checked like `optional` or `Expected`)
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:267
StructuredBindingPolicy = TypeHintPolicy;
StructuredBindingPolicy.PrintCanonicalTypes = true;
}
----------------
zyounan wrote:
> `PrintCanonicalTypes` turns on printing default template arguments, which would prevent the patch from printing, e.g., `std::basic_string<char>`, within the default length limitation.
>
> ```
> std::map<std::string, int> Map;
>
> for (auto &[Key, Value] : Map) // Key: basic_string<char, char_traits<char>, allocator<char>>, whose length exceeds the default threshold.
>
> ```
>
> Is it safe to drop it now? I believe this patch can handle the case the comment mentioned.
Unfortunately, I don't think it works in general. I tried commenting out this line and
`TypeHints.StructuredBindings_TupleLike` failed.
(Note, this was with the `BuiltinType` heuristic removed. If we keep the `BuilinType` heuristic, a modified version of the testcase (e.g. struct members are changed from `int` to a class type) still fails.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151785/new/
https://reviews.llvm.org/D151785
More information about the cfe-commits
mailing list