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

add wil::create_file_create, create_file_open, etc. tame use of CreateFileW #382

Merged
merged 20 commits into from
Nov 8, 2023

Conversation

ChrisGuzak
Copy link
Member

@ChrisGuzak ChrisGuzak commented Oct 24, 2023

I distilled the documented behavior of CreateFileW into these helpers with the goal enabling simple use of the functionality without having to construct the long and complicated, and interdependent parameter list.

auto handle = wil::open_file(path.c_str());

The design uses "open", "create" and "trucate" to distinguish the cases that open existing files and use that creates and modifies existing files. Here is more complicated use that demonstrates the handle and error code being returned (used in success cases, so providing access to it is essential.).

auto path = wil::ExpandEnvironmentStringsW<std::wstring>(LR"(%TEMP%\create_file_test.txt)");
auto [fileHandle, error] = wil::try_create_new_file(path.c_str())

Here, are the method names that intend to describe the semantics.

auto handle = wil::create_new_file(path.c_str());
auto handle = wil::open_or_create_file(path.c_str());
auto handle = wil::open_or_truncate_existing_file(path.c_str());
auto handle = wil::truncate_existing_file(path.c_str());

PR includes tests now.

@ChrisGuzak ChrisGuzak changed the title add wil::create_file_create, create_file_open, etc. tame use of Win32 CreateFile add wil::create_file_create, create_file_open, etc. tame use of CreateFileW Oct 24, 2023
Copy link
Member

@jonwis jonwis left a comment

Choose a reason for hiding this comment

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

I'm of multiple minds on this. I like that it's clearing up the "floor wax and dessert topping" form of the API, I don't know that this is the way that's most intuitive in naming for me yet.

include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
include/wil/filesystem.h Outdated Show resolved Hide resolved
@@ -650,7 +649,6 @@ TEST_CASE("FileSystemTests::GetFileInfo<FileStreamInfo>", "[filesystem]")
wistd::unique_ptr<FILE_STREAM_INFO> streamInfo;
auto hr = wil::GetFileInfoNoThrow<FileStreamInfo>(handle.get(), streamInfo);
REQUIRE(hr == S_OK);
#endif
}

TEST_CASE("FileSystemTests::QueryFullProcessImageNameW", "[filesystem]")
Copy link
Member

Choose a reason for hiding this comment

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

nit: Not critical, but this could be pulled out of the ifdef for exception support

Copy link
Member

@dunhor dunhor left a comment

Choose a reason for hiding this comment

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

I'm still of the opinion that flags & attributes should come before security attributes, but otherwise things look good to me

@ChrisGuzak ChrisGuzak merged commit ce501f7 into master Nov 8, 2023
2 checks passed
@ChrisGuzak ChrisGuzak deleted the user/chrisg/create_file branch May 21, 2024 05:14
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

4 participants