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 File::Delete and File::DeleteDir warning flags #9307

Conversation

Dentomologist
Copy link
Contributor

Adds a flag to File::Delete and File::DeleteDir dictating whether they emit console warnings when the specified path doesn't exist. The flags are true by default to maintain existing behavior.

Also modifies the return value of DeleteDir when attempting to delete a nonexistent path to better reflect the intention of the function (nothing's there after the call). This change doesn't break anything because none of the existing callers actually check the return value.

Adds tests for Delete and DeleteDir.

Makes File::DeleteDir return true when attempting to delete a
nonexistent path.

The purpose of DeleteDir is to ensure the path doesn't exist after the
call, which is better reflected by the new return value. Additionally,
none of the current callers actually check the return value so this
won't break any existing code.
@lioncash
Copy link
Member

lioncash commented Dec 3, 2020

I don't think a bool is particularly useful visually here, because

File::Delete(path, false); doesn't really communicate intent particularly well. Maybe consider using an enum class instead.

@Dentomologist
Copy link
Contributor Author

Dentomologist commented Dec 3, 2020

I don't think a bool is particularly useful visually here, because

File::Delete(path, false); doesn't really communicate intent particularly well. Maybe consider using an enum class instead.

I considered that but was reluctant to add an enum class that was only used by two function parameters. If you feel that's a better route I can go ahead and do that.

@lioncash
Copy link
Member

lioncash commented Dec 3, 2020

Please do. It's much better to be noisily informed about behavior/intent, than not being informed about it at all.

@Dentomologist Dentomologist force-pushed the add-deleted-file-missing-warning-flag branch from 6256777 to 1ba40d3 Compare December 3, 2020 23:46
@Dentomologist
Copy link
Contributor Author

Noted! I've added the enum class as requested.

Adds a flag to File::Delete and File::DeleteDir functions to control
whether a console warning is emitted when the file or directory doesn't
exist. The flag is optional and true by default to match current behavior.
@Dentomologist Dentomologist force-pushed the add-deleted-file-missing-warning-flag branch from 1ba40d3 to 76251aa Compare December 6, 2020 00:22
@Dentomologist Dentomologist force-pushed the add-deleted-file-missing-warning-flag branch from 76251aa to b35ba42 Compare December 10, 2020 07:37
@Dentomologist
Copy link
Contributor Author

I refactored a few things in the unit tests so that could use another look. In addition to renaming the path variables to use the m_ prefix I also separated assertions into separate functions. This is intended make it clearer exactly what is being tested, and also that the results of the delete shouldn't depend on the value of the IfAbsentBehavior.

Source/UnitTests/FileUtil.cpp Outdated Show resolved Hide resolved
Source/UnitTests/FileUtil.cpp Outdated Show resolved Hide resolved
Source/UnitTests/FileUtil.cpp Outdated Show resolved Hide resolved
Source/UnitTests/FileUtil.cpp Outdated Show resolved Hide resolved
@Dentomologist Dentomologist force-pushed the add-deleted-file-missing-warning-flag branch from b35ba42 to 760e7e6 Compare December 10, 2020 17:50
@Dentomologist
Copy link
Contributor Author

Added suggested changes by @leoetlino

@leoetlino leoetlino merged commit 2e63cc8 into dolphin-emu:master Dec 11, 2020
@Dentomologist Dentomologist deleted the add-deleted-file-missing-warning-flag branch December 11, 2020 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants