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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Sorry for losing this.

> LMK if you think it makes sense to move some of this logic in AST.cpp (clangd). objcContainerLocalScope seems like it would be useful to generalize support for objc container decls by ensuring that categories becoming fully qualified with their class name

Yes, please move these to AST.cpp, maybe rename to `printObjCMethod` and `printObjCContainer`.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:64
 
+static llvm::StringRef getNameForObjCInterface(const ObjCInterfaceDecl *ID) {
+  return ID ? ID->getName() : "<<error-type>>";
----------------
this function's name doesn't really describe what it's for

it has 3 callsites and is only needed in 2, just inline it?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:71
+  llvm::raw_string_ostream OS(Name);
+  const ObjCInterfaceDecl *Class = Method->getClassInterface();
+
----------------
looking at the implementation, this is null iff the method is part of a protocol.
<<error-type>> doesn't seem appropriatefor that case


================
Comment at: clang-tools-extra/clangd/Hover.cpp:78
+          dyn_cast<ObjCCategoryImplDecl>(Method->getDeclContext()))
+    OS << '(' << *CID << ')';
+
----------------
prefer CID->getName(), the operator<< is really surprising here


================
Comment at: clang-tools-extra/clangd/Hover.cpp:80
+
+  OS << ' ' << Method->getSelector().getAsString();
+  if (Method->isVariadic())
----------------
nit: Method->getSelector().print(OS << ' ');

(yeah, the implementation is dumb, but maybe they'll fix it)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:85
+  OS << ']';
+  return Name;
+}
----------------
you're neither flushing nor destroying the OS, this is fragile.

(From playing with godbolt, it looks like this works as long as move-elision kicks in, but adding `if (false) return ""` at the top prevents the stream from being flushed here)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:96
+  }
+  if (const ObjCCategoryImplDecl *CI = dyn_cast<ObjCCategoryImplDecl>(C)) {
+    std::string Name;
----------------
can't you return objcContainerLocalScope(CI->getCategoryDecl()) ?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:149
+  // and instead support local scopes.
+  if (isa<ObjCMethodDecl>(DC) || isa<ObjCContainerDecl>(DC))
+    return "";
----------------
nit: isa<ObjCMethodDecl, ObjCContainerDecl>(DC)

I'm not sure the comment adds much here (particularly "we return an empty string") - maybe remove?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68590/new/

https://reviews.llvm.org/D68590