Apply format only if --format is specified #79466
Merged
Conversation
This file contains hidden or 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
clang-apply-replacements used to apply format even without --format is specified. This because, methods like createReplacementsForHeaders only takes the Spec.Style and would re-order the headers even when it was not requested. The fix is to set up Spec.Style only if --format is provided. Also added note to ReleaseNotes.rst Based on llvm#70801
|
@llvm/pr-subscribers-clang-tools-extra Author: Dmitry Polukhin (dmpolukhin) Changesclang-apply-replacements used to apply format even without --format is specified. This because, methods like createReplacementsForHeaders only takes the Spec.Style and would re-order the headers even when it was not requested. The fix is to set up Spec.Style only if --format is provided. Also added note to ReleaseNotes.rst Based on #70801 Full diff: https://github.com/llvm/llvm-project/pull/79466.diff 7 Files Affected:
diff --git a/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp b/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
index 9331898bf2570e6..68b5743c6540f8a 100644
--- a/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ b/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -141,9 +141,9 @@ int main(int argc, char **argv) {
tooling::ApplyChangesSpec Spec;
Spec.Cleanup = true;
- Spec.Style = FormatStyle;
Spec.Format = DoFormat ? tooling::ApplyChangesSpec::kAll
: tooling::ApplyChangesSpec::kNone;
+ Spec.Style = DoFormat ? FormatStyle : format::getNoStyle();
for (const auto &FileChange : Changes) {
FileEntryRef Entry = FileChange.first;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index fd2cba4e4f463be..228c9c176232161 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -109,6 +109,13 @@ Changes in existing checks
Removed checks
^^^^^^^^^^^^^^
+Miscellaneous
+^^^^^^^^^^^^^
+
+- Fixed incorrect apply format in clang-apply-repalcements when no `--format`
+ option is specified. Now clang-apply-repalcements applies format only with
+ the option.
+
Improvements to include-fixer
-----------------------------
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/no.cpp b/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/no.cpp
new file mode 100644
index 000000000000000..140e266200b72f4
--- /dev/null
+++ b/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/no.cpp
@@ -0,0 +1,10 @@
+#include <string>
+// CHECK: #include <string>
+// CHECK-NEXT: #include <memory>
+// CHECK-NEXT: #include "bar.h"
+#include <memory>
+#include "foo.h"
+#include "bar.h"
+
+void foo() {
+}
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/no.yaml b/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/no.yaml
new file mode 100644
index 000000000000000..172b5ee1f1ac5a1
--- /dev/null
+++ b/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/no.yaml
@@ -0,0 +1,14 @@
+---
+MainSourceFile: no.cpp
+Diagnostics:
+ - DiagnosticName: test-header-format
+ DiagnosticMessage:
+ Message: Fix
+ FilePath: $(path)/no.cpp
+ FileOffset: 36
+ Replacements:
+ - FilePath: $(path)/no.cpp
+ Offset: 36
+ Length: 17
+ ReplacementText: ""
+...
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/yes.cpp b/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/yes.cpp
new file mode 100644
index 000000000000000..20e6b90c39b6393
--- /dev/null
+++ b/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/yes.cpp
@@ -0,0 +1,10 @@
+#include <string>
+// CHECK: #include "bar.h"
+// CHECK-NEXT: #include <memory>
+// CHECK-NEXT: #include <string>
+#include <memory>
+#include "foo.h"
+#include "bar.h"
+
+void foo() {
+}
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/yes.yaml b/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/yes.yaml
new file mode 100644
index 000000000000000..723c4c5d5ceb391
--- /dev/null
+++ b/clang-tools-extra/test/clang-apply-replacements/Inputs/format_header/yes.yaml
@@ -0,0 +1,14 @@
+---
+MainSourceFile: yes.cpp
+Diagnostics:
+ - DiagnosticName: test-header-format
+ DiagnosticMessage:
+ Message: Fix
+ FilePath: $(path)/yes.cpp
+ FileOffset: 36
+ Replacements:
+ - FilePath: $(path)/yes.cpp
+ Offset: 36
+ Length: 17
+ ReplacementText: ""
+...
diff --git a/clang-tools-extra/test/clang-apply-replacements/format-header.cpp b/clang-tools-extra/test/clang-apply-replacements/format-header.cpp
new file mode 100644
index 000000000000000..6a221c44b06a78f
--- /dev/null
+++ b/clang-tools-extra/test/clang-apply-replacements/format-header.cpp
@@ -0,0 +1,13 @@
+// RUN: mkdir -p %T/Inputs/format_header_yes
+// RUN: mkdir -p %T/Inputs/format_header_no
+//
+//
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/format_header/yes.cpp > %T/Inputs/format_header_yes/yes.cpp
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/format_header/no.cpp > %T/Inputs/format_header_no/no.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/format_header_yes#" %S/Inputs/format_header/yes.yaml > %T/Inputs/format_header_yes/yes.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/format_header_no#" %S/Inputs/format_header/no.yaml > %T/Inputs/format_header_no/no.yaml
+// RUN: clang-apply-replacements -format -style="{BasedOnStyle: llvm, SortIncludes: CaseSensitive}" %T/Inputs/format_header_yes
+// RUN: clang-apply-replacements %T/Inputs/format_header_no
+// RUN: FileCheck --strict-whitespace -input-file=%T/Inputs/format_header_yes/yes.cpp %S/Inputs/format_header/yes.cpp
+// RUN: FileCheck --strict-whitespace -input-file=%T/Inputs/format_header_no/no.cpp %S/Inputs/format_header/no.cpp
+//
|
|
@AaronBallman @bcardosolopes please take a look. |
AaronBallman
approved these changes
Feb 1, 2024
Co-authored-by: Aaron Ballman <aaron@aaronballman.com>
AaronBallman
approved these changes
Feb 2, 2024
agozillon
pushed a commit
to agozillon/llvm-project
that referenced
this pull request
Feb 5, 2024
clang-apply-replacements used to apply format even without --format is specified. This because, methods like createReplacementsForHeaders only takes the Spec.Style and would re-order the headers even when it was not requested. The fix is to set up Spec.Style only if --format is provided. Also added note to ReleaseNotes.rst Based on llvm#70801 --------- Co-authored-by: Kugan <34810920+kuganv@users.noreply.github.com> Co-authored-by: Aaron Ballman <aaron@aaronballman.com>
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.
clang-apply-replacements used to apply format even without --format is specified. This because, methods like createReplacementsForHeaders only takes the Spec.Style and would re-order the headers even when it was not requested. The fix is to set up Spec.Style only if --format is provided.
Also added note to ReleaseNotes.rst
Based on #70801