[PATCH] D82436: [clangd] Implement textDocument/foldingRange

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I'd suggest splitting out the DocumentSymbol changes into a separate patch from the FoldingRanges patches.



================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:140
   SourceLocation NameLoc = nameLocation(ND, SM);
   // getFileLoc is a good choice for us, but we also need to make sure
   // sourceLocToPosition won't switch files, so we call getSpellingLoc on top of
----------------
this whole block seems likely to be obsolete if you switch to toHalfOpenFileRange


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:167
   SI.range =
       Range{sourceLocToPosition(SM, BeginLoc), sourceLocToPosition(SM, EndLoc)};
+  // Include trailing '}' token for most symbol kinds.
----------------
As far as I can tell, this line is just always wrong: EndLoc points at the *start* of the last token in the range, so the last token will be omitted from the range in all cases.


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:168
       Range{sourceLocToPosition(SM, BeginLoc), sourceLocToPosition(SM, EndLoc)};
+  // Include trailing '}' token for most symbol kinds.
+  if (SK == SymbolKind::Class || SK == SymbolKind::Struct ||
----------------
I can't see any conceptual reason for this to be conditional.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.h:28
 
+llvm::Expected<std::vector<FoldingRange>> getFoldingRanges(ParsedAST &AST);
+
----------------
Can we still have a high-level description of what this function does?

```
Returns a list of ranges whose contents might be collapsible in an editor.
This should include large scopes, preprocessor blocks etc.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82436