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