zyounan added a comment. Thank you very much for the insightful review! Regarding to your suggestion,
> We have an existing heuristic here, should we move the logic into > maybeDesugar() so it's all in one place? once we merge these two functions, the overloaded `addTypeHint` without a PrintingPolicy parameter will merely remains one line for forwarding. As we're going to use a single `PrintingPolicy` after D152520 <https://reviews.llvm.org/D152520>, I think it would be better to eventually drop the parameter `PrintingPolicy` in `addTypeHint` in that patch. ================ 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 ---------------- nridge wrote: > 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? Good catch! I looked up the standard and found out that pointers or references are of compound types, and that 'qualified' usually goes along with 'const or volatile'. ================ Comment at: clang-tools-extra/clangd/InlayHints.cpp:201 + // type. Peel it. + QualType Last = QT; + for (QT = QT->getPointeeType(); !QT.isNull(); ---------------- nridge wrote: > 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 Rephrased as the suggestion. (TBH, I didn't realize storing the Next instead of the Prev could make it more terse. Thanks for the hint!) ================ Comment at: clang-tools-extra/clangd/InlayHints.cpp:207 + }; + for (QualType Desugared = + PeelQualifier(QT->getLocallyUnqualifiedSingleStepDesugaredType()); ---------------- nridge wrote: > 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; > } > ``` Cool. ================ Comment at: clang-tools-extra/clangd/InlayHints.cpp:215 + return true; + if (Desugared->getAs<BuiltinType>()) + return true; ---------------- nridge wrote: > 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? Not that I could think of except that case. It's possible that `size_type` varies across different platforms, so it seems okay to remove it. ================ Comment at: clang-tools-extra/clangd/InlayHints.cpp:235 +// parameter QT. +QualType tryDesugar(ASTContext &AST, QualType QT) { + if (isDependentOrTrivialSugaredType(QT)) ---------------- nridge wrote: > name suggestion: `maybeDesugar` > > (`try` sounds like it might fail and return something that needs to be > checked like `optional` or `Expected`) Frankly speaking I used to mix up these two and just learned the differences. Thank you! ================ Comment at: clang-tools-extra/clangd/InlayHints.cpp:267 StructuredBindingPolicy = TypeHintPolicy; StructuredBindingPolicy.PrintCanonicalTypes = true; } ---------------- nridge wrote: > zyounan wrote: > > nridge wrote: > > > 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.) > > > > > Thank you for providing me with the case. I think the flag > > `PrintCanonicalTypes` actually controls two aspects of styles, if I > > understand TypePrinter correctly: > > > > 1. For type aliases (a.k.a. typedefs and using alias), the 'canonical' type > > (i.e., the "most" desugared type) is [[ > > https://searchfox.org/llvm/rev/3a458256ee22a0e7c31529de42fa6caa263d88fe/clang/lib/AST/TypePrinter.cpp#179 > > | obtained ]] before printing. > > > > 2. For template arguments, print [[ > > https://searchfox.org/llvm/rev/3a458256ee22a0e7c31529de42fa6caa263d88fe/clang/lib/AST/TypePrinter.cpp#2158 > > | default parameters ]] even they weren't specified. > > > > For 1, we could achieve the same goal at the caller site: For > > `DecompositionDecl`, instead of creating another different printing policy, > > call `QualType::getCanonicalType()` to get the QualType to be passed into > > `addTypeHint`. I did a modification and found that test cases from > > `TypeHints` are all passed even we disable > > `StructuredBindingPolicy.PrintCanonicalTypes`. > > > > For 2, I think for most of the cases, printing default template parameters > > is an unexpected side effect. (I didn't see any test cases requiring > > default template parameters on structure bindings, but please correct me if > > I'm missing something.) > > > > What do you think? I'd like to submit another review addressing this if it > > looks fine to you. > > What do you think? I'd like to submit another review addressing this if it > > looks fine to you. > > +1, it sounds reasonable to me Link to the [[ https://reviews.llvm.org/D152520 | proposed review ]]. 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
