[PATCH] D147395: [Clangd] Make the type hint length limit configurable

[Yi Zhang via Phabricator via cfe-commits]

zhangyi1357 added a comment.

I dont have commit access. Could you help committing the change for me? @hokein



================
Comment at: clang-tools-extra/clangd/Config.h:151
+    // Limit the length of type names in inlay hints.
+    size_t TypeNameLimit = 32;
   } InlayHints;
----------------
hokein wrote:
> zhangyi1357 wrote:
> > hokein wrote:
> > > I would extend it a bit more -- 0 means no limit. 
> > > 
> > > Can you also add a unittest in `TEST(TypeHints, LongTypeName)` in `InlayHintTests.cpp`? 
> > > 0 means no limit.
> > This is quite a good idea. I've done it.
> > 
> > For unittest, there is already `TEST(TypeHints, LongTypeName)` in `InlayHintTests.cpp`. Do you mean add more tests in the same `TEST` or another `TEST` with TypeNameLimit configured?
> > 
> I mean adding one more test in the same `TEST(TypeHints, LongTypeName)`.
> 
> This test verifies the the long type name is shown when the limit is set to 0, something like
> 
> ```
> TEST(TypeHints, LongTypeName) {
>   assertTypeHints(R"cpp(
>     template <typename, typename, typename>
>     struct A {};
>     struct MultipleWords {};
>     A<MultipleWords, MultipleWords, MultipleWords> foo();
>     // Omit type hint past a certain length (currently 32)
>     auto var = foo();
>   )cpp");
> 
>     Config cfg; 
>      ... // set the limit to 0
> 
>    assertTypeHints(R"cpp(
>     template <typename, typename, typename>
>     struct A {};
>     struct MultipleWords {};
>     A<MultipleWords, MultipleWords, MultipleWords> foo();
>     // Omit type hint past a certain length (currently 32)
>     auto var = foo();
>   )cpp", ExpectedHint...);
> }
> ```
Thanks for your example! I've added that.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:80
   C.InlayHints.Designators = false;
+  C.InlayHints.TypeNameLimit = 1;
   return C;
----------------
hokein wrote:
> why do we need this change? I think it should be fine without it.
Yes, without this line the tests will not fail. But I am a little confused about the code below without 'C.InlayHints.TypeNameLimit = 1;'.
```
Config noHintsConfig() {
  Config C;
  C.InlayHints.Parameters = false;
  C.InlayHints.DeducedTypes = false;
  C.InlayHints.Designators = false;
  // C.InlayHints.TypeNameLimit = 1;
  return C;
}

template <typename... ExpectedHints>
void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
                 ExpectedHints... Expected) {
  Annotations Source(AnnotatedSource);
  TestTU TU = TestTU::withCode(Source.code());
  TU.ExtraArgs.push_back("-std=c++20");
  auto AST = TU.build();

  EXPECT_THAT(hintsOfKind(AST, Kind),
              ElementsAre(HintMatcher(Expected, Source)...));
  // Sneak in a cross-cutting check that hints are disabled by config.
  // We'll hit an assertion failure if addInlayHint still gets called.
  WithContextValue WithCfg(Config::Key, noHintsConfig());
  EXPECT_THAT(inlayHints(AST, std::nullopt), IsEmpty());  // Why does this succeed with TypeNameLimit = 32 ?
}

TEST(TypeHints, Smoke) {
  assertTypeHints(R"cpp(
    auto $waldo[[waldo]] = 42;
  )cpp",
                  ExpectedHint{": int", "waldo"});
}
```
The dault TypeNameLimit is 32, why does the second `EXPECT_THAT` succeed with TypeNameLimit = 32.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147395