sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Cool!



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:40
+    // types would be "tuple_element<I, A>::type".
+    TypeHintPolicy.PrintCanonicalTypes = true;
   }
----------------
nridge wrote:
> While playing around with this, it did occur to me that in some cases it's 
> more helpful to print sugared types than canonical types, for example in a 
> case like this:
> 
> ```
> template <typename, typename, typename>
> struct SomeLongType {};
> 
> using ShortType = SomeLongType<int, float, double>;
> 
> ShortType func();
> 
> auto x = func();  // would prefer "ShortType" as hint
> ```
> 
> However, it turns out that `AutoType` doesn't retain the sugared type to 
> begin with (there's a FIXME about that 
> [here](https://searchfox.org/llvm/rev/e497b12a69604b6d691312a30f6b86da4f18f7f8/clang/include/clang/AST/Type.h#4953)),
>  so setting `PrintCanonicalTypes=true` doesn't actually regress cases like 
> this (they were already printing the canonical type).
This makes sense - can you add a comment with a short version of this?
// Often we'd prefer sugar types for `auto`, but the AST doesn't retain them 
anyway.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:466
+  // Hint individual bindings, not the aggregate.
   assertTypeHints(R"cpp(
+    // 1. Struct with public fields.
----------------
since we have the handy assertTypeHints function, can we split this into 
several separate tests?
(Or one table-driven test so at least the test code + corresponding assertions 
are next to each other)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104617/new/

https://reviews.llvm.org/D104617

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to