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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
