[PATCH] D151785: [clangd] Desugar dependent type aliases for auto type hints

[Nathan Ridge via Phabricator via cfe-commits]

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