kadircet marked 10 inline comments as done.
kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:567
+ const HoverInfo Expected;
+ } // namespace
+ Cases[] = {
----------------
sammccall wrote:
> namespace?!
>
> might be a clang-format bug?
I must have messed the brackets at some point :D
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:574
+ )cpp",
+ {/*NamespaceScope=*/std::string(""),
+ /*LocalScope=*/std::string(""),
----------------
sammccall wrote:
> using the constructor/aggregate init here has a couple of downsides:
> - vebosity: you need to list every field, even those that are not set for
> this case (TemplateParameters), or are misleading (SymRange is none)
> - brittleness: it makes it hard to change the struct at all
> - readability: the /*foo=*/ comments aren't bad, but not ideal either
>
> Using field-by-field initialization would be better I think, though it's a
> bit awkward here.
>
> but e.g. you could make the member std::function<HoverInfo> ExpectedBuilder,
> and write
>
> ```
>
> [&](HoverInfo &Expected) {
> Expected.Name = "foo";
> Expected.Kind = SymbolKind::Function;
> ...
> }
> ```
yeah that looks a lot better, thanks!
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:592
+ )cpp",
+ {/*NamespaceScope=*/std::string("ns1::ns2"),
+ /*LocalScope=*/std::string(""),
----------------
sammccall wrote:
> I think this should be "ns1::ns2::", as we use scope internally.
> This means we can simply concatenate parts to form a qname.
>
> For the current rendering, it's easy to strip ::
should it also be "::" for global namespace ? which would also result in
prefixing any symbol in global namespace with "::" when printing.
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:599
+ /*Definition=*/"void foo()",
+ /*Type=*/llvm::None,
+ /*ReturnType=*/std::string("void"),
----------------
sammccall wrote:
> It would be nice to add `void()` or `void (&)()` or so if it's easy.
> This is what `:YcmCompleter GetType` would show
just put the type without any parameter names, but I am not sure whether users
would want that. I believe people find current hover behavior a lot more useful
then just showing type(which is done by libclang)
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:782
+ /*Definition=*/
+ "auto lamb = [&bar](int T, bool B) -> bool {\n return T && B && "
+ "bar;\n}",
----------------
sammccall wrote:
> is it easy to omit the body?
yes, sent out D62487
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:951
)cpp",
- "Declared in namespace ns1\n\nstruct MyClass {}",
+ "Declared in ns1\n\nstruct MyClass {}",
},
----------------
sammccall wrote:
> note that if you want to it's easy to keep "Declared in namespace" - the
> LocalScope is empty.
> (Though we can't provide this info for non-namespace containers)
I believe it doesn't make sense to add only one, so leaving it until we hear
from users that they need the kind information
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61497/new/
https://reviews.llvm.org/D61497
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
