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

[Younan Zhang via Phabricator via cfe-commits]

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