[clangd] NFC: Update FIXME comment regarding lack of c/dtor support

[kbobyrev]

Summary

Both SymbolKind and indexSymbolKindToSymbolKind support constructors and
separate them into a different category from regular methods.

[njames93]

clang-tools-extra/clangd/FindSymbols.cpp

182–183

[kbobyrev]

Fix typo.

clang-tools-extra/clangd/FindSymbols.cpp

182–183

Oops, thanks!

[kadircet]

Am I missing something? We still have:

case index::SymbolKind::Constructor:
case index::SymbolKind::Destructor:
  return SymbolKind::Constructor;

in Protocol.cpp. E.g. Constructors and Destructors are still classified badly. I suppose the bit around they're all methods are wrong though, maybe just drop that bit ?

[kbobyrev]

In D89935#2346648, @kadircet wrote:

Am I missing something? We still have:

case index::SymbolKind::Constructor:
case index::SymbolKind::Destructor:
  return SymbolKind::Constructor;

in Protocol.cpp. E.g. Constructors and Destructors are still classified badly. I suppose the bit around they're all methods are wrong though, maybe just drop that bit ?

Yeah but the LSP SymbolKind which we are converting to does not have a destructor type, same thing with CompletionItemKind, so I guess we really do treat ctors and dtors the same way from the LSP perspective, aren't we?

[kadircet]

Yeah but the LSP SymbolKind which we are converting to does not have a destructor type, same thing with CompletionItemKind, so I guess we really do treat ctors and dtors the same way from the LSP perspective, aren't we?

Yes, and that's what the previous fixme was saying. Now we are dropping that bit, but the code is still behaving badly?

[kbobyrev]

I thought the point of the comment was us not handling it properly rather than LSP not supporting it (e.g. LSP does support Operator but we do not). Then, the comment about ctor and dtor being indistinguishable probably belongs to Protocol.h/cpp and SymbolKind there in particular?

[kadircet]

I thought the point of the comment was us not handling it properly rather than LSP not supporting it (e.g. LSP does support Operator but we do not). Then, the comment about ctor and dtor being indistinguishable probably belongs to Protocol.h/cpp and SymbolKind there in particular?

If that was the case I would expect this comment to be above previous line, i.e. index::getSymbolInfo, as that's the one doing the clang-level mapping. Whereas indexSymbolKindToSymbolKind does LSP mapping.

[nridge]

Note, I've cargo-culted this comment into declToTypeHierarchyItem() as well, so whatever change we make here should be made there too.

[kbobyrev]

Only drop the bits about "they're all considered methods", update XRefs.cpp.

[kbobyrev]

Rebase on top of master.

clang-tools-extra/clangd/FindSymbols.cpp

llvm::Optional<DocumentSymbol> declToSym(ASTContext &Ctx, const NamedDecl &ND) {
   SourceLocation BeginLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getBeginLoc()));
   SourceLocation EndLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getEndLoc()));
   const auto SymbolRange =
       toHalfOpenFileRange(SM, Ctx.getLangOpts(), {BeginLoc, EndLoc});
   if (!SymbolRange)
     return llvm::None;
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //        correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   DocumentSymbol SI;
   SI.name = printName(Ctx, ND);
   SI.kind = SK;
   SI.deprecated = ND.isDeprecated();
   SI.range = Range{sourceLocToPosition(SM, SymbolRange->getBegin()),
                    sourceLocToPosition(SM, SymbolRange->getEnd())};

clang-tools-extra/clangd/XRefs.cpp

declToTypeHierarchyItem(ASTContext &Ctx, const NamedDecl &ND) {
   if (!FilePath || !TUPath)
     return llvm::None; // Not useful without a uri.
 
   Position NameBegin = sourceLocToPosition(SM, NameLoc);
   Position NameEnd = sourceLocToPosition(
       SM, Lexer::getLocForEndOfToken(NameLoc, 0, SM, Ctx.getLangOpts()));
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //        correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   TypeHierarchyItem THI;
   THI.name = printName(Ctx, ND);
   THI.kind = SK;
   THI.deprecated = ND.isDeprecated();
   THI.range = Range{sourceLocToPosition(SM, DeclRange->getBegin()),
                     sourceLocToPosition(SM, DeclRange->getEnd())};