[PATCH] D68590: [clangd] Improve hover support for Objective-C

[Sam McCall via Phabricator via cfe-commits]

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