[clangd] Fix: exclude insertions at end of CursorPlaceholder in formatting #87746
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be 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 If you have received no comments on your PR for a week, you can request a review 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
added
clang
|
@llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-tools-extra Author: roife (roife) ChangesFixes #71983 This commit introduces a new param 'includeInsAtPos' for 'getShiftedCodePosition', which is defaulted to be true. If 'includeInsAtPos' is true, the insertion at the 'Position' will not be counted in. Changes made:
Full diff: https://github.com/llvm/llvm-project/pull/87746.diff 4 Files Affected:
diff --git a/clang-tools-extra/clangd/Format.cpp b/clang-tools-extra/clangd/Format.cpp
index 272a34d4ed7972..901a709e2e3934 100644
--- a/clang-tools-extra/clangd/Format.cpp
+++ b/clang-tools-extra/clangd/Format.cpp
@@ -341,7 +341,7 @@ formatIncremental(llvm::StringRef OriginalCode, unsigned OriginalCursor,
unsigned FormattedCursorStart =
FormattingChanges.getShiftedCodePosition(Cursor),
FormattedCursorEnd = FormattingChanges.getShiftedCodePosition(
- Cursor + Incremental.CursorPlaceholder.size());
+ Cursor + Incremental.CursorPlaceholder.size(), false);
tooling::Replacements RemoveCursorPlaceholder(
tooling::Replacement(Filename, FormattedCursorStart,
FormattedCursorEnd - FormattedCursorStart, ""));
diff --git a/clang-tools-extra/clangd/unittests/FormatTests.cpp b/clang-tools-extra/clangd/unittests/FormatTests.cpp
index f7384a1bc63c99..22c19fefc61aad 100644
--- a/clang-tools-extra/clangd/unittests/FormatTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FormatTests.cpp
@@ -315,6 +315,25 @@ vector<int> x = {1, 2, 3}^
)cpp");
}
+TEST(FormatIncremental, InsertBraces) {
+ format::FormatStyle Style =
+ format::getGoogleStyle(format::FormatStyle::LK_Cpp);
+ Style.InsertBraces = true;
+ expectAfterNewline(R"cpp(
+int main() {
+ while (true)
+^
+}
+)cpp",
+ R"cpp(
+int main() {
+ while (true) {
+
+ }^
+}
+)cpp",
+ Style);
+}
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang/include/clang/Tooling/Core/Replacement.h b/clang/include/clang/Tooling/Core/Replacement.h
index f9452111e147f1..69fe5186dfb0a5 100644
--- a/clang/include/clang/Tooling/Core/Replacement.h
+++ b/clang/include/clang/Tooling/Core/Replacement.h
@@ -268,9 +268,10 @@ class Replacements {
std::vector<Range> getAffectedRanges() const;
// Returns the new offset in the code after replacements being applied.
- // Note that if there is an insertion at Offset in the current replacements,
- // \p Offset will be shifted to Offset + Length in inserted text.
- unsigned getShiftedCodePosition(unsigned Position) const;
+ // If \p includeInsAtPos is true and there is an insertion at Offset in the
+ // current replacements, \p Offset will be shifted to Offset + Length in
+ // inserted text. Otherwise, the insertion at Offset will not be counted in.
+ unsigned getShiftedCodePosition(unsigned Position, bool includeInsAtPos = true) const;
unsigned size() const { return Replaces.size(); }
diff --git a/clang/lib/Tooling/Core/Replacement.cpp b/clang/lib/Tooling/Core/Replacement.cpp
index 269f17a6db4cfc..95a3b62f47c708 100644
--- a/clang/lib/Tooling/Core/Replacement.cpp
+++ b/clang/lib/Tooling/Core/Replacement.cpp
@@ -544,10 +544,13 @@ std::vector<Range> Replacements::getAffectedRanges() const {
return combineAndSortRanges(ChangedRanges);
}
-unsigned Replacements::getShiftedCodePosition(unsigned Position) const {
+unsigned Replacements::getShiftedCodePosition(unsigned Position,
+ bool includeInsAtPos) const {
unsigned Offset = 0;
for (const auto &R : Replaces) {
- if (R.getOffset() + R.getLength() <= Position) {
+ unsigned End = R.getOffset() + R.getLength();
+ if (End <= Position
+ && (includeInsAtPos || (End < Position || R.getLength() > 0))) {
Offset += R.getReplacementText().size() - R.getLength();
continue;
}
|
roife
force-pushed
the
fix-issue-71983
branch
2 times, most recently
from
eabedb3
to
03fe6be
Compare
|
I'm not sure why the test is showing as failed since all tests of clangd have passed; I didn't find any failed tests in the log. |
You can test this locally with the following command:git-clang-format --diff d4cd65ecf2546e509f43363f96364c976f49b9da 03fe6bec808cb968c35f1bad7432ea6155ed0115 -- clang-tools-extra/clangd/Format.cpp clang-tools-extra/clangd/unittests/FormatTests.cpp clang/include/clang/Tooling/Core/Replacement.h clang/lib/Tooling/Core/Replacement.cppView the diff from clang-format here.diff --git a/clang-tools-extra/clangd/unittests/FormatTests.cpp b/clang-tools-extra/clangd/unittests/FormatTests.cpp
index 22c19fefc6..72e662a070 100644
--- a/clang-tools-extra/clangd/unittests/FormatTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FormatTests.cpp
@@ -317,7 +317,7 @@ vector<int> x = {1, 2, 3}^
TEST(FormatIncremental, InsertBraces) {
format::FormatStyle Style =
- format::getGoogleStyle(format::FormatStyle::LK_Cpp);
+ format::getGoogleStyle(format::FormatStyle::LK_Cpp);
Style.InsertBraces = true;
expectAfterNewline(R"cpp(
int main() {
diff --git a/clang/include/clang/Tooling/Core/Replacement.h b/clang/include/clang/Tooling/Core/Replacement.h
index 69fe5186df..d2b79e4ab0 100644
--- a/clang/include/clang/Tooling/Core/Replacement.h
+++ b/clang/include/clang/Tooling/Core/Replacement.h
@@ -271,7 +271,8 @@ public:
// If \p includeInsAtPos is true and there is an insertion at Offset in the
// current replacements, \p Offset will be shifted to Offset + Length in
// inserted text. Otherwise, the insertion at Offset will not be counted in.
- unsigned getShiftedCodePosition(unsigned Position, bool includeInsAtPos = true) const;
+ unsigned getShiftedCodePosition(unsigned Position,
+ bool includeInsAtPos = true) const;
unsigned size() const { return Replaces.size(); }
diff --git a/clang/lib/Tooling/Core/Replacement.cpp b/clang/lib/Tooling/Core/Replacement.cpp
index 95a3b62f47..07d3e4e587 100644
--- a/clang/lib/Tooling/Core/Replacement.cpp
+++ b/clang/lib/Tooling/Core/Replacement.cpp
@@ -549,8 +549,8 @@ unsigned Replacements::getShiftedCodePosition(unsigned Position,
unsigned Offset = 0;
for (const auto &R : Replaces) {
unsigned End = R.getOffset() + R.getLength();
- if (End <= Position
- && (includeInsAtPos || (End < Position || R.getLength() > 0))) {
+ if (End <= Position &&
+ (includeInsAtPos || (End < Position || R.getLength() > 0))) {
Offset += R.getReplacementText().size() - R.getLength();
continue;
}
|
|
AFAIK the Windows CI has been rather fragile recently, and I think you could probably ignore it. (And here is the error log, which appears unrelated to this patch.) |
This commit introduces a new param 'includeInsAtPos' for 'getShiftedCodePosition', which is defaulted to be true. If 'includeInsAtPos' is true, the insertion at the 'Position' will be counted in. Otherwise, the insertion will not be counted in. Changes made: - Added new param 'includeInsAtPos' to 'getShiftedCodePosition'. - Set 'includeInsAtPos' to be false when calculate the end position of 'CursorPlaceholder' in 'formatIncremental' of clangd. Fixes llvm#71983
|
ping |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #71983
This commit introduces a new param 'includeInsAtPos' for 'getShiftedCodePosition', which is defaulted to be true. If 'includeInsAtPos' is true, the insertion at the 'Position' will not be counted in.
Changes made: