Skip to content

[clangd] Implement simple folding of preprocessor branches #80592

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

Closed
wants to merge 1 commit into from

Conversation

144026
Copy link

@144026 144026 commented Feb 4, 2024

Extract directive branches information from DirectiveTree, fold branches that don't end with eof.

Fixes clangd/clangd#1661

Examples:

Folded:
image

Unfolded:
image

Folded (unpaired conditionals):
image

Unfolded (unpaired conditionals):
image

Copy link

github-actions bot commented Feb 4, 2024

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot
Copy link
Member

llvmbot commented Feb 4, 2024

@llvm/pr-subscribers-clangd

Author: Ruihua Dong (144026)

Changes

Extract directive branches information from DirectiveTree, fold branches that don't end with eof.

Fixes clangd/clangd#1661

Examples:

Folded:
<img width="400" alt="image" src="https://github.com/llvm/llvm-project/assets/44112701/c5b7ab39-7371-47f8-9d0d-29f11dbc9a1f">

Unfolded:
<img width="400" alt="image" src="https://github.com/llvm/llvm-project/assets/44112701/6bfcb325-26b1-4435-9e3d-f2deed8136ef">

Folded (unpaired conditionals):
<img width="400" alt="image" src="https://github.com/llvm/llvm-project/assets/44112701/fdddfcf8-866f-4ab7-b8a7-6848f9bee0de">

Unfolded (unpaired conditionals):
<img width="400" alt="image" src="https://github.com/llvm/llvm-project/assets/44112701/090a9f31-69d7-41de-ba45-54f86196da9f">


Full diff: https://github.com/llvm/llvm-project/pull/80592.diff

3 Files Affected:

  • (modified) clang-tools-extra/clangd/SemanticSelection.cpp (+18)
  • (modified) clang-tools-extra/pseudo/include/clang-pseudo/DirectiveTree.h (+3)
  • (modified) clang-tools-extra/pseudo/lib/DirectiveTree.cpp (+54)
diff --git a/clang-tools-extra/clangd/SemanticSelection.cpp b/clang-tools-extra/clangd/SemanticSelection.cpp
index 3d687173b2be9..d2aefb51fd49a 100644
--- a/clang-tools-extra/clangd/SemanticSelection.cpp
+++ b/clang-tools-extra/clangd/SemanticSelection.cpp
@@ -220,6 +220,24 @@ getFoldingRanges(const std::string &Code, bool LineFoldingOnly) {
   auto EndPosition = [&](const pseudo::Token &T) {
     return offsetToPosition(Code, EndOffset(T));
   };
+
+  // Preprocessor directives
+  auto PPRanges = pseudo::pairDirectiveRanges(DirectiveStructure, OrigStream);
+  for (const auto &R : PPRanges) {
+    auto BTok = OrigStream.tokens()[R.Begin];
+    auto ETok = OrigStream.tokens()[R.End];
+    if (ETok.Kind == tok::eof)
+      continue;
+    if (BTok.Line >= ETok.Line)
+      continue;
+
+    Position Start = EndPosition(BTok);
+    Position End = StartPosition(ETok);
+    if (LineFoldingOnly)
+      End.line--;
+    AddFoldingRange(Start, End, FoldingRange::REGION_KIND);
+  }
+
   auto Tokens = ParseableStream.tokens();
   // Brackets.
   for (const auto &Tok : Tokens) {
diff --git a/clang-tools-extra/pseudo/include/clang-pseudo/DirectiveTree.h b/clang-tools-extra/pseudo/include/clang-pseudo/DirectiveTree.h
index 2b6cb63297915..2e5f007f94a76 100644
--- a/clang-tools-extra/pseudo/include/clang-pseudo/DirectiveTree.h
+++ b/clang-tools-extra/pseudo/include/clang-pseudo/DirectiveTree.h
@@ -124,6 +124,9 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &,
 /// The choices are stored in Conditional::Taken nodes.
 void chooseConditionalBranches(DirectiveTree &, const TokenStream &Code);
 
+std::vector<Token::Range> pairDirectiveRanges(const DirectiveTree &Tree,
+                                              const TokenStream &Code);
+
 } // namespace pseudo
 } // namespace clang
 
diff --git a/clang-tools-extra/pseudo/lib/DirectiveTree.cpp b/clang-tools-extra/pseudo/lib/DirectiveTree.cpp
index 9e853e46edc23..4882f01890654 100644
--- a/clang-tools-extra/pseudo/lib/DirectiveTree.cpp
+++ b/clang-tools-extra/pseudo/lib/DirectiveTree.cpp
@@ -353,5 +353,59 @@ TokenStream DirectiveTree::stripDirectives(const TokenStream &In) const {
   return Out;
 }
 
+namespace {
+class RangePairer {
+  std::vector<Token::Range> &Ranges;
+
+public:
+  RangePairer(std::vector<Token::Range> &Ranges) : Ranges(Ranges) {}
+
+  void walk(const DirectiveTree &T) {
+    for (const auto &C : T.Chunks)
+      std::visit(*this, C);
+  }
+
+  void operator()(const DirectiveTree::Code &C) {}
+
+  void operator()(const DirectiveTree::Directive &) {}
+
+  void operator()(const DirectiveTree::Conditional &C) {
+    Token::Range Range;
+    Token::Index Last;
+    auto First = true;
+    for (const auto &B : C.Branches) {
+      if (First) {
+        First = false;
+      } else {
+        Range = {Last, B.first.Tokens.Begin};
+        Ranges.push_back(Range);
+      }
+      Last = B.first.Tokens.Begin;
+    }
+    Range = {Last, C.End.Tokens.Begin};
+    Ranges.push_back(Range);
+
+    for (const auto &B : C.Branches)
+      walk(B.second);
+  }
+};
+} // namespace
+
+std::vector<Token::Range> pairDirectiveRanges(const DirectiveTree &Tree,
+                                              const TokenStream &Code) {
+  std::vector<Token::Range> Ranges;
+  RangePairer(Ranges).walk(Tree);
+
+  // Transform paired ranges to start with last token in its logical line
+  for (auto &R : Ranges) {
+    const Token *Tok = &Code.tokens()[R.Begin + 1];
+    while (Tok->Kind != tok::eof && !Tok->flag(LexFlags::StartsPPLine))
+      ++Tok;
+    Tok = Tok - 1;
+    R.Begin = Tok->OriginalIndex;
+  }
+  return std::move(Ranges);
+}
+
 } // namespace pseudo
 } // namespace clang

@llvmbot
Copy link
Member

llvmbot commented Feb 4, 2024

@llvm/pr-subscribers-clang-tools-extra

Author: Ruihua Dong (144026)

Changes

Extract directive branches information from DirectiveTree, fold branches that don't end with eof.

Fixes clangd/clangd#1661

Examples:

Folded:
<img width="400" alt="image" src="https://github.com/llvm/llvm-project/assets/44112701/c5b7ab39-7371-47f8-9d0d-29f11dbc9a1f">

Unfolded:
<img width="400" alt="image" src="https://github.com/llvm/llvm-project/assets/44112701/6bfcb325-26b1-4435-9e3d-f2deed8136ef">

Folded (unpaired conditionals):
<img width="400" alt="image" src="https://github.com/llvm/llvm-project/assets/44112701/fdddfcf8-866f-4ab7-b8a7-6848f9bee0de">

Unfolded (unpaired conditionals):
<img width="400" alt="image" src="https://github.com/llvm/llvm-project/assets/44112701/090a9f31-69d7-41de-ba45-54f86196da9f">


Full diff: https://github.com/llvm/llvm-project/pull/80592.diff

3 Files Affected:

  • (modified) clang-tools-extra/clangd/SemanticSelection.cpp (+18)
  • (modified) clang-tools-extra/pseudo/include/clang-pseudo/DirectiveTree.h (+3)
  • (modified) clang-tools-extra/pseudo/lib/DirectiveTree.cpp (+54)
diff --git a/clang-tools-extra/clangd/SemanticSelection.cpp b/clang-tools-extra/clangd/SemanticSelection.cpp
index 3d687173b2be9..d2aefb51fd49a 100644
--- a/clang-tools-extra/clangd/SemanticSelection.cpp
+++ b/clang-tools-extra/clangd/SemanticSelection.cpp
@@ -220,6 +220,24 @@ getFoldingRanges(const std::string &Code, bool LineFoldingOnly) {
   auto EndPosition = [&](const pseudo::Token &T) {
     return offsetToPosition(Code, EndOffset(T));
   };
+
+  // Preprocessor directives
+  auto PPRanges = pseudo::pairDirectiveRanges(DirectiveStructure, OrigStream);
+  for (const auto &R : PPRanges) {
+    auto BTok = OrigStream.tokens()[R.Begin];
+    auto ETok = OrigStream.tokens()[R.End];
+    if (ETok.Kind == tok::eof)
+      continue;
+    if (BTok.Line >= ETok.Line)
+      continue;
+
+    Position Start = EndPosition(BTok);
+    Position End = StartPosition(ETok);
+    if (LineFoldingOnly)
+      End.line--;
+    AddFoldingRange(Start, End, FoldingRange::REGION_KIND);
+  }
+
   auto Tokens = ParseableStream.tokens();
   // Brackets.
   for (const auto &Tok : Tokens) {
diff --git a/clang-tools-extra/pseudo/include/clang-pseudo/DirectiveTree.h b/clang-tools-extra/pseudo/include/clang-pseudo/DirectiveTree.h
index 2b6cb63297915..2e5f007f94a76 100644
--- a/clang-tools-extra/pseudo/include/clang-pseudo/DirectiveTree.h
+++ b/clang-tools-extra/pseudo/include/clang-pseudo/DirectiveTree.h
@@ -124,6 +124,9 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &,
 /// The choices are stored in Conditional::Taken nodes.
 void chooseConditionalBranches(DirectiveTree &, const TokenStream &Code);
 
+std::vector<Token::Range> pairDirectiveRanges(const DirectiveTree &Tree,
+                                              const TokenStream &Code);
+
 } // namespace pseudo
 } // namespace clang
 
diff --git a/clang-tools-extra/pseudo/lib/DirectiveTree.cpp b/clang-tools-extra/pseudo/lib/DirectiveTree.cpp
index 9e853e46edc23..4882f01890654 100644
--- a/clang-tools-extra/pseudo/lib/DirectiveTree.cpp
+++ b/clang-tools-extra/pseudo/lib/DirectiveTree.cpp
@@ -353,5 +353,59 @@ TokenStream DirectiveTree::stripDirectives(const TokenStream &In) const {
   return Out;
 }
 
+namespace {
+class RangePairer {
+  std::vector<Token::Range> &Ranges;
+
+public:
+  RangePairer(std::vector<Token::Range> &Ranges) : Ranges(Ranges) {}
+
+  void walk(const DirectiveTree &T) {
+    for (const auto &C : T.Chunks)
+      std::visit(*this, C);
+  }
+
+  void operator()(const DirectiveTree::Code &C) {}
+
+  void operator()(const DirectiveTree::Directive &) {}
+
+  void operator()(const DirectiveTree::Conditional &C) {
+    Token::Range Range;
+    Token::Index Last;
+    auto First = true;
+    for (const auto &B : C.Branches) {
+      if (First) {
+        First = false;
+      } else {
+        Range = {Last, B.first.Tokens.Begin};
+        Ranges.push_back(Range);
+      }
+      Last = B.first.Tokens.Begin;
+    }
+    Range = {Last, C.End.Tokens.Begin};
+    Ranges.push_back(Range);
+
+    for (const auto &B : C.Branches)
+      walk(B.second);
+  }
+};
+} // namespace
+
+std::vector<Token::Range> pairDirectiveRanges(const DirectiveTree &Tree,
+                                              const TokenStream &Code) {
+  std::vector<Token::Range> Ranges;
+  RangePairer(Ranges).walk(Tree);
+
+  // Transform paired ranges to start with last token in its logical line
+  for (auto &R : Ranges) {
+    const Token *Tok = &Code.tokens()[R.Begin + 1];
+    while (Tok->Kind != tok::eof && !Tok->flag(LexFlags::StartsPPLine))
+      ++Tok;
+    Tok = Tok - 1;
+    R.Begin = Tok->OriginalIndex;
+  }
+  return std::move(Ranges);
+}
+
 } // namespace pseudo
 } // namespace clang

@zyn0217
Copy link
Contributor

zyn0217 commented Feb 4, 2024

Note that clang-pseudo is deprecated and will soon get removed from the repository. I have no idea about the replacement, and I'm not sure if it makes sense to evolve the feature with it.

I invited some folks (who may be in charge of clang-pseudo) for visibility.

@144026
Copy link
Author

144026 commented Feb 4, 2024

@zyn0217 Thanks for the information, seems like a completed removal of pseudo parser has been decided in this discourse thread, and shifting back to AST parser for foldingRange support would only lose a marginal multi-line comment folding feature.

Dependency of this patch hasn't been touched yet in #80081 , so I tested a rebased build of this patch on #80081 on my Linux box, which still could pass and work.

I understand that evolving this end-of-life parser only adds maintenance burden and should be avoided, but I hope there will be some ways (may be in the original AST parser?) to support PP conditional folding, especially when reading complex headers.

@144026
Copy link
Author

144026 commented Feb 4, 2024

The build pipeline failure is on Windows x64, and appears to be some random error irrelevant to this patch:

MT: command "C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\x64\mt.exe /nologo /manifest bin\dexp.exe.manifest /outputresource:bin\dexp.exe;#1" failed (exit code 0x1f) with the following output:
mt.exe : general error c101008d: Failed to write the updated manifest to the resource of file "bin\dexp.exe". Operation did not complete successfully because the file contains a virus or potentially unwanted software.

@144026
Copy link
Author

144026 commented Feb 22, 2024

Any updates on pseudo parser removal? It seems like clangd/vscode-clangd#297 is open for pp folding as well, and people have been using vscode-explicit-folding as a workaround.
@sam-mccall Would it be possible to apply and keep pp folding support before foldingRange support is put back on AST parser?

Extract directive branches information from DirectiveTree, fold branches
that don't end with eof.

Fixes clangd/clangd#1661
@torshepherd
Copy link
Contributor

Darn, this was just closed last week as I put up an issue #110390 😭

Clang-pseudo was just removed: #80081 (comment)

Is the plan to refactor now and re-put this up?

@aaronliu0130
Copy link

I'm new to this, but from what I see, this PR could be easily put up simply removing the pseudo:: prefixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Precompiled macro #ifndef/#if/#elif/#else/#endif areas do not code fold
5 participants