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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
