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

[include-cleaner] Fix a race issue when editing multiple files. #76960

Merged
merged 4 commits into from
Jan 5, 2024

Conversation

hokein
Copy link
Collaborator

@hokein hokein commented Jan 4, 2024

We have a previous fix be861b6, which snapshots all processing files.

It works most of times, the snapshot (InMemoryFileSystem) is based on the file path. The file-path-based lookup can fail in a subtle way for some tricky cases (we encounter it internally), which will result in reading a corrupted file.

This is a different fix, we don't modify files on the fly, instead, we write files when the tool finishes for all files.

We have a previous fix llvm@be861b6,
which snapshots all processing files.

It works most of times, the snapshot (InMemoryFileSystem) is based on
the file path. The file-path-based lookup can fail in a subtle way for
some tricky cases (we encounter it internally), which will result in
reading a corrupted header.

This is a different fix, we don't modify files on the fly, instead,
we write files when the tool finishes analysises for all files.
@hokein hokein requested a review from kadircet January 4, 2024 14:14
Copy link

github-actions bot commented Jan 4, 2024

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

Copy link
Member

@kadircet kadircet left a comment

Choose a reason for hiding this comment

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

thanks, lgtm!

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp Outdated Show resolved Hide resolved
@hokein hokein merged commit 67963d3 into llvm:main Jan 5, 2024
4 checks passed
@hokein hokein deleted the race-multi-files branch January 5, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants