sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Thanks, I think you're right about all of this. Implementation looks OK too. Please add TODOs for the cases where we're punting to later vs deciding not to fix. In D61497#1512003 <https://reviews.llvm.org/D61497#1512003>, @kadircet wrote: > - Behavior on lambdas, currently it just keeps the old behaviour. I believe > they should look more like functions. Do you think it is necessary for first > patch? Agree. Not necessary in first patch IMO. Add a TODO though. > - Template classes/functions, what we have in definition is not great but I > don't think it is too much of a problem since semantic representation carries > all the useful information. These look OK to me, though examples are complicated and I might be missing something. > - Declared in #SOME_KIND# #SOME_PLACE#, currently we drop SOME_KIND and it > needs additional info to be propogated since it is not the type of the symbol > but rather it is immediate container. I don't think it is necessary for > initial version. This seems OK to me. As noted you can keep it in the case that it's "namespace". If you want to propagate the container kind in this patch that's also fine. > - Discrepencies between hovering over auto/decltype and explicitly spelled > types, again this keeps the existing behaviour. You can see details in > testcases. Testcases look pretty good I think. ================ Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:567 + const HoverInfo Expected; + } // namespace + Cases[] = { ---------------- namespace?! might be a clang-format bug? ================ Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:574 + )cpp", + {/*NamespaceScope=*/std::string(""), + /*LocalScope=*/std::string(""), ---------------- 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; ... } ``` ================ Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:592 + )cpp", + {/*NamespaceScope=*/std::string("ns1::ns2"), + /*LocalScope=*/std::string(""), ---------------- 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 :: ================ Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:599 + /*Definition=*/"void foo()", + /*Type=*/llvm::None, + /*ReturnType=*/std::string("void"), ---------------- It would be nice to add `void()` or `void (&)()` or so if it's easy. This is what `:YcmCompleter GetType` would show ================ Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:678 + /*TemplateParameters=*/llvm::None}}, + // Class template + {R"cpp( ---------------- can we have a class template example where the instantiation isn't templated? e.g. ``` template<typename T> class vector{}; [[vector]]<int> X; ``` (just because this is an example that we've discussed a lot, and these edge-case tests make it hard to see the common behavior) ================ 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}", ---------------- is it easy to omit the body? ================ Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:951 )cpp", - "Declared in namespace ns1\n\nstruct MyClass {}", + "Declared in ns1\n\nstruct MyClass {}", }, ---------------- 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) 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
