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

Fix fstream.seekp(0, ios::cur) #3773

Merged
merged 22 commits into from
Jun 22, 2023
Merged

Fix fstream.seekp(0, ios::cur) #3773

merged 22 commits into from
Jun 22, 2023

Conversation

achabense
Copy link
Contributor

Fixes #3572. As discovered in the issue, we need fseek for side effects even when (off,way)==(0,ios::cur).

@achabense achabense requested a review from a team as a code owner June 15, 2023 16:41
@achabense achabense changed the title Fix for fstream.seekp(0, ios::cur) Fix fstream.seekp(0, ios::cur) Jun 15, 2023
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.

@strega-nil-ms

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@CaseyCarter CaseyCarter added the bug Something isn't working label Jun 15, 2023
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

The fix looks fine, but I'd like to see a regression test (a test that fails without the fix but succeeds with it) to validate it.

@achabense
Copy link
Contributor Author

achabense commented Jun 16, 2023

I have no idea how to deal with this formatting problem... horrible experience

@StephanTLavavej
Copy link
Member

Please note that we strongly recommend configuring your editor to clang-format on save. (VSCode can do this with Microsoft's C/C++ Extension or xaver's clang-format extension, among others.) The CI system is a shared resource, so it's best to avoid putting unnecessary load on it.

@StephanTLavavej StephanTLavavej self-assigned this Jun 19, 2023
@achabense achabense requested a review from CaseyCarter June 20, 2023 09:29
@StephanTLavavej
Copy link
Member

Thanks for fixing this bug and testing the fix! 😻 I validated and pushed minor cleanups to the test.

@CaseyCarter
Copy link
Contributor

Please note that we strongly recommend configuring your editor to clang-format on save.

There's also a cmake format target that clang-formats everything, but format-on-save is both impossible to forget and (generally) faster since the cmake target literally clang-formats everything.

Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

I'll push a change to run the test in C++14 mode.

tests/std/tests/GH_003572_fstream_seekp_0_cur/env.lst Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter removed their assignment Jun 22, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jun 22, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 518f449 into microsoft:main Jun 22, 2023
@StephanTLavavej
Copy link
Member

Thanks again for this correctness fix! 🐞 ✅ 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<fstream>: seeking 0 with std::ios::cur ignored with bidirectional streams
4 participants