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

Use permissive wide-to-narrow transcoding in filesystem_error #1010

Merged

Conversation

Chronial
Copy link
Contributor

@Chronial Chronial commented Jul 7, 2020

Fixes #927: std::filesystem_error constructor used to raise a std::system_error when given paths that are not representable in the current codepage. This replaces unrepresentable characters with a replacement character instead of raising system_error.

@Chronial Chronial requested a review from a team as a code owner July 7, 2020 13:19
@ghost
Copy link

ghost commented Jul 7, 2020

CLA assistant check
All CLA requirements met.

@Chronial Chronial force-pushed the master_feature/fs_error_encoding branch from 0307330 to d9baf83 Compare July 7, 2020 15:40
@Chronial Chronial force-pushed the master_feature/fs_error_encoding branch from d9baf83 to b83cd85 Compare July 7, 2020 15:58
@BillyONeal
Copy link
Member

This looks reasonable but needs tests.

@BillyONeal
Copy link
Member

I don't know why the CI is unhappy.

This looks reasonable but needs tests.

For example:

#include <assert.h>
#include <filesystem>
#include <string_view>
#include <system_error>

using namespace std;
namespace fs = std::filesystem;

void test_filesystem_error_with_bad_codepage_characters() {
    fs::path problem_path{L"\U0001F4A3"}; // an emoji that goes boom
    fs::filesystem_error err{"something bad happened", problem_path, error_code{}};
    assert(string_view{err.what()}.find('?') != string_view::npos);
}

int main() { test_filesystem_error_with_bad_codepage_characters(); }

@Chronial
Copy link
Contributor Author

Chronial commented Jul 7, 2020

Thanks, I'll add a test. Is it ok to assume that the tests won't run with a utf-8 system codepage? Not that I have any idea what else to do about that :).

@BillyONeal
Copy link
Member

Thanks, I'll add a test. Is it ok to assume that the tests won't run with a utf-8 system codepage? Not that I have any idea what else to do about that :).

If I understand correctly getting that requires a manifest we aren't going to be adding to our bits. If you're concerned you can replace my 'emoji that goes boom' with an incorrect surrogate pair or something like that.

@Chronial Chronial changed the title Fix path conversion to narrow encoding in filesystem_error <filesystem> Fix path conversion to narrow encoding in filesystem_error Jul 8, 2020
@Chronial
Copy link
Contributor Author

Chronial commented Jul 8, 2020

Added a test. I am not sure whether WideCharToMultiByte will always use a ? as replacement character – according to the docs, that's up to the codepage to specify. I think that's fine, so I rather checked to make sure that the rest of the path is still there.

I also added a check to make sure the test is actually testing something.

@Chronial

This comment has been minimized.

@StephanTLavavej

This comment has been minimized.

@cbezault

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Jul 8, 2020
@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

This looks good to me - I have some superficial nitpicks, but all of the logic appears to be correct and following our various conventions (thanks for imitating the existing machinery closely).

stl/inc/filesystem Outdated Show resolved Hide resolved
stl/src/filesystem.cpp Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Yay! Thanks for clang-formatting - looks like my brain can't perfectly simulate it yet.

@Chronial
Copy link
Contributor Author

Thanks for your quick feedback 👍

@CaseyCarter CaseyCarter self-assigned this Jul 16, 2020
@CaseyCarter CaseyCarter changed the title <filesystem> Fix path conversion to narrow encoding in filesystem_error Use permissive wide-to-narrow transcoding in filesystem_error Jul 16, 2020
@CaseyCarter CaseyCarter merged commit 0965ada into microsoft:master Jul 17, 2020
@CaseyCarter
Copy link
Member

Congratulations on your first STL contribution!

@CaseyCarter CaseyCarter removed their assignment Jul 17, 2020
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.

<filesystem>: system_error thrown from copy_file
5 participants