[PATCH] D150683: [clangd] Tweak "provides" hover card when symbols have the same name

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1539
+        Front, [&](llvm::StringRef Sym) { P.appendCode(Sym); },
+        [&] { P.appendText(", "); });
     if (UsedSymbolNames.size() > Front.size()) {
----------------
VitaNuo wrote:
> What will this do if `Front` has an even number of elements? AFAIU the message will end in a comma then, which is not desirable.
llvm::interleave invokes the first callback for each element, and the second callback between subsequent elements. So this prints Front as a comma-separated list.
(It's very similar to the example in the llvm::interleave docs)


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3492
          HI.Name = "foo.h";
-         HI.UsedSymbolNames = {"Foo", "Bar", "Baz"};
+         HI.UsedSymbolNames = {"Foo", "Bar", "Bar"};
        },
----------------
VitaNuo wrote:
> Why did you introduce this change? This is just a rendering test, and you seem to be feeding with input that should not be possible anymore (after this patch).
The rendering logic was incorrect: `if (Sym != Front.back())` was intending to test whether this was the last element, but instead was testing if it had the same text as the last element.

After fixing that bug I realized we probably didn't ever want to include overloads separately anyway, independent of any rendering issues.

But the rendering logic should still be fixed! (render() doesn't know there are no duplicates!)
And when fixing it I think it makes sense to test it - but I don't feel strongly, I can revert the test if you prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150683