-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[clangd] Remove the unused AST-based code folding Implementation. #166189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: Haojian Wu (hokein) ChangesIn clangd, we use the non-ast version one. Full diff: https://github.com/llvm/llvm-project/pull/166189.diff 3 Files Affected:
diff --git a/clang-tools-extra/clangd/SemanticSelection.cpp b/clang-tools-extra/clangd/SemanticSelection.cpp
index 3353121a01825..91e221bc853d5 100644
--- a/clang-tools-extra/clangd/SemanticSelection.cpp
+++ b/clang-tools-extra/clangd/SemanticSelection.cpp
@@ -41,72 +41,6 @@ void addIfDistinct(const Range &R, std::vector<Range> &Result) {
}
}
-std::optional<FoldingRange> toFoldingRange(SourceRange SR,
- const SourceManager &SM) {
- const auto Begin = SM.getDecomposedLoc(SR.getBegin()),
- End = SM.getDecomposedLoc(SR.getEnd());
- // Do not produce folding ranges if either range ends is not within the main
- // file. Macros have their own FileID so this also checks if locations are not
- // within the macros.
- if ((Begin.first != SM.getMainFileID()) || (End.first != SM.getMainFileID()))
- return std::nullopt;
- FoldingRange Range;
- Range.startCharacter = SM.getColumnNumber(Begin.first, Begin.second) - 1;
- Range.startLine = SM.getLineNumber(Begin.first, Begin.second) - 1;
- Range.endCharacter = SM.getColumnNumber(End.first, End.second) - 1;
- Range.endLine = SM.getLineNumber(End.first, End.second) - 1;
- return Range;
-}
-
-std::optional<FoldingRange>
-extractFoldingRange(const syntax::Node *Node,
- const syntax::TokenBufferTokenManager &TM) {
- if (const auto *Stmt = dyn_cast<syntax::CompoundStatement>(Node)) {
- const auto *LBrace = cast_or_null<syntax::Leaf>(
- Stmt->findChild(syntax::NodeRole::OpenParen));
- // FIXME(kirillbobyrev): This should find the last child. Compound
- // statements have only one pair of braces so this is valid but for other
- // node kinds it might not be correct.
- const auto *RBrace = cast_or_null<syntax::Leaf>(
- Stmt->findChild(syntax::NodeRole::CloseParen));
- if (!LBrace || !RBrace)
- return std::nullopt;
- // Fold the entire range within braces, including whitespace.
- const SourceLocation LBraceLocInfo =
- TM.getToken(LBrace->getTokenKey())->endLocation(),
- RBraceLocInfo =
- TM.getToken(RBrace->getTokenKey())->location();
- auto Range = toFoldingRange(SourceRange(LBraceLocInfo, RBraceLocInfo),
- TM.sourceManager());
- // Do not generate folding range for compound statements without any
- // nodes and newlines.
- if (Range && Range->startLine != Range->endLine)
- return Range;
- }
- return std::nullopt;
-}
-
-// Traverse the tree and collect folding ranges along the way.
-std::vector<FoldingRange>
-collectFoldingRanges(const syntax::Node *Root,
- const syntax::TokenBufferTokenManager &TM) {
- std::queue<const syntax::Node *> Nodes;
- Nodes.push(Root);
- std::vector<FoldingRange> Result;
- while (!Nodes.empty()) {
- const syntax::Node *Node = Nodes.front();
- Nodes.pop();
- const auto Range = extractFoldingRange(Node, TM);
- if (Range)
- Result.push_back(*Range);
- if (const auto *T = dyn_cast<syntax::Tree>(Node))
- for (const auto *NextNode = T->getFirstChild(); NextNode;
- NextNode = NextNode->getNextSibling())
- Nodes.push(NextNode);
- }
- return Result;
-}
-
} // namespace
llvm::Expected<SelectionRange> getSemanticRanges(ParsedAST &AST, Position Pos) {
@@ -163,18 +97,6 @@ llvm::Expected<SelectionRange> getSemanticRanges(ParsedAST &AST, Position Pos) {
return std::move(Head);
}
-// FIXME(kirillbobyrev): Collect comments, PP conditional regions, includes and
-// other code regions (e.g. public/private/protected sections of classes,
-// control flow statement bodies).
-// Related issue: https://github.com/clangd/clangd/issues/310
-llvm::Expected<std::vector<FoldingRange>> getFoldingRanges(ParsedAST &AST) {
- syntax::Arena A;
- syntax::TokenBufferTokenManager TM(AST.getTokens(), AST.getLangOpts(),
- AST.getSourceManager());
- const auto *SyntaxTree = syntax::buildSyntaxTree(A, TM, AST.getASTContext());
- return collectFoldingRanges(SyntaxTree, TM);
-}
-
// FIXME( usaxena95): Collect includes and other code regions (e.g.
// public/private/protected sections of classes, control flow statement bodies).
// Related issue: https://github.com/clangd/clangd/issues/310
diff --git a/clang-tools-extra/clangd/SemanticSelection.h b/clang-tools-extra/clangd/SemanticSelection.h
index dd9d3ea7e81d3..4e19397a0307a 100644
--- a/clang-tools-extra/clangd/SemanticSelection.h
+++ b/clang-tools-extra/clangd/SemanticSelection.h
@@ -26,10 +26,6 @@ namespace clangd {
/// If pos is not in any interesting range, return [Pos, Pos).
llvm::Expected<SelectionRange> getSemanticRanges(ParsedAST &AST, Position Pos);
-/// Returns a list of ranges whose contents might be collapsible in an editor.
-/// This should include large scopes, preprocessor blocks etc.
-llvm::Expected<std::vector<FoldingRange>> getFoldingRanges(ParsedAST &AST);
-
/// Returns a list of ranges whose contents might be collapsible in an editor.
/// This version uses the pseudoparser which does not require the AST.
llvm::Expected<std::vector<FoldingRange>>
diff --git a/clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp b/clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
index 4efae25dcd077..9e8b603c73dbd 100644
--- a/clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -196,76 +196,6 @@ TEST(SemanticSelection, RunViaClangdServer) {
ElementsAre(SourceAnnotations.range("empty")));
}
-TEST(FoldingRanges, ASTAll) {
- const char *Tests[] = {
- R"cpp(
- #define FOO int foo() {\
- int Variable = 42; \
- return 0; \
- }
-
- // Do not generate folding range for braces within macro expansion.
- FOO
-
- // Do not generate folding range within macro arguments.
- #define FUNCTOR(functor) functor
- void func() {[[
- FUNCTOR([](){});
- ]]}
-
- // Do not generate folding range with a brace coming from macro.
- #define LBRACE {
- void bar() LBRACE
- int X = 42;
- }
- )cpp",
- R"cpp(
- void func() {[[
- int Variable = 100;
-
- if (Variable > 5) {[[
- Variable += 42;
- ]]} else if (Variable++)
- ++Variable;
- else {[[
- Variable--;
- ]]}
-
- // Do not generate FoldingRange for empty CompoundStmts.
- for (;;) {}
-
- // If there are newlines between {}, we should generate one.
- for (;;) {[[
-
- ]]}
- ]]}
- )cpp",
- R"cpp(
- class Foo {
- public:
- Foo() {[[
- int X = 1;
- ]]}
-
- private:
- int getBar() {[[
- return 42;
- ]]}
-
- // Braces are located at the same line: no folding range here.
- void getFooBar() { }
- };
- )cpp",
- };
- for (const char *Test : Tests) {
- auto T = Annotations(Test);
- auto AST = TestTU::withCode(T.code()).build();
- EXPECT_THAT(gatherFoldingRanges(llvm::cantFail(getFoldingRanges(AST))),
- UnorderedElementsAreArray(T.ranges()))
- << Test;
- }
-}
-
TEST(FoldingRanges, PseudoParserWithoutLineFoldings) {
const char *Tests[] = {
R"cpp(
|
HighCommander4
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @aketchum15 since you brought up the possibility of using this AST folding code again in this Discourse thread. Do you have any short-term plans to do so?
If not, I'd say we can go ahead with this removal for now, and we can always bring it back later if we decide.
Since we haven't heard back, I'd say we can go ahead with this for now. As mentioned we can restore the code later if we choose. |
773510e to
73881e4
Compare
In clangd, we use the non-ast version one.