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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
