zyounan added inline comments.
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:267
StructuredBindingPolicy = TypeHintPolicy;
StructuredBindingPolicy.PrintCanonicalTypes = true;
}
----------------
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.
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
