sammccall added a comment.
Thanks! This generally looks good, just need to find the right home for some of
the logic.
================
Comment at: clangd/XRefs.cpp:455
+static std::string getNameForObjCMethod(const ObjCMethodDecl *Method) {
+ std::string Name;
----------------
XRefs.cpp isn't really the right place for this.
If there isn't somewhere appropriate in clangAST, then making this a case of
printName in AST.h is probably best.
It should have unit tests (rather than testing it all through hover)
================
Comment at: clangd/XRefs.cpp:532
+ // Don't consider Type/Function/ObjC decls to be a namespace. Instead, check
+ // where they themselves are declared by recursing.
----------------
Maybe "skip over Type/Function/ObjC methods, these are part of the local scope
instead".
(Not sure about "objc decl" as it seems to cover a lot of other things too)
================
Comment at: clangd/unittests/XRefsTests.cpp:1029
+TEST(Hover, ObjectiveC) {
+ struct {
----------------
combine test cases with above.
Do we need so many? I think 2 plus some unit-tests of the "pretty-print a decl"
code should be enough
================
Comment at: clangd/unittests/XRefsTests.cpp:1177
+ };
+ for (const auto &Case : Cases) {
+ SCOPED_TRACE(Case.Code);
----------------
you've got a loop here but only one test case. Combine with the other loop or
just assert directly?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68590/new/
https://reviews.llvm.org/D68590
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
