Skip to content
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

[clang-tidy] Fix result check after overwriteChangedFiles() #86360

Merged
merged 1 commit into from
Mar 23, 2024

Conversation

mikerice1969
Copy link
Contributor

If any return from overwriteChangedFiles is true some fixes were not applied.

If any return from overwriteChangedFiles is true some fixes were not
applied.
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 22, 2024

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

@llvm/pr-subscribers-clang-tidy

Author: Mike Rice (mikerice1969)

Changes

If any return from overwriteChangedFiles is true some fixes were not applied.


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

1 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/ClangTidy.cpp (+1-1)
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 40ac6918faf407..b877ea06dc05cd 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -233,7 +233,7 @@ class ErrorReporter {
         if (!tooling::applyAllReplacements(Replacements.get(), Rewrite)) {
           llvm::errs() << "Can't apply replacements for file " << File << "\n";
         }
-        AnyNotWritten &= Rewrite.overwriteChangedFiles();
+        AnyNotWritten |= Rewrite.overwriteChangedFiles();
       }
 
       if (AnyNotWritten) {

Copy link

✅ With the latest revision this PR passed the Python code formatter.

Copy link

✅ With the latest revision this PR passed the C/C++ code formatter.

@jansvoboda11
Copy link
Contributor

This could use a test.

@mikerice1969
Copy link
Contributor Author

This could use a test.

I thought about it, but I have no idea how to write that test. Do you have an idea?

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Yes looks like previously AnyNotWritten coudn't be true.

I don't see need for test / release notes.
After all the only thing this impact is a print.

@PiotrZSL PiotrZSL merged commit d7ce6b4 into llvm:main Mar 23, 2024
7 checks passed
@mikerice1969 mikerice1969 deleted the clang-tidy-any-not-written branch March 25, 2024 15:27
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.

None yet

4 participants