Refactor Filesystem Code for Clarity

[AZero13]

This makes the code more readable as well as lowers operations to code paths that actually need them

[StephanTLavavej]

Thanks for looking into improving the codebase!

As a maintainer, I find this PR to be concerning. Every code change is risky to some degree (even innocent-seeming changes are occasionally risky, up to and including ABI breaks), and also costs time to review. With a brief glance at the changes, I can see that there's some good stuff here (e.g. I particularly like the use of the SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE constant which is now available), but I have these specific concerns:

1. This is a "grab bag" of different refactorings, which is harder to review. A PR that performs only one kind of transformation (e.g. De Morgan changes) is significantly less risky and easier to review - even when many sites are being changed. The dangerous thing about mixed changes is that we can nod our heads to a bunch of safe changes and miss the one scary/wrong change in the middle.
   • It can sometimes be acceptable to have one PR with several kinds of refactorings cleanly grouped into separate commits. I suppose I would accept separate commits here.
2. Substantial behavior changes (whether bug fixes or performance improvements) should not be mixed with stylistic refactorings - that is, <filesystem>: Resize files in a better way #2767 should be addressed in an isolated PR. Occasionally, because we are human (and slightly lazy), it is okay to perform one tiny cleanup in a PR that's otherwise dedicated to fixing a bug or improving performance. However, any substantial mixing really must be avoided. (Same rationale as avoiding mixed refactorings, but even stronger.)
   • Putting behavior changes and refactorings into separate commits doesn't solve this concern, they really need to be separate PRs.
3. It would be helpful for your PR description to explain why each change is being made. For some changes, I can immediately see the clarity improvement (e.g. De Morganing "test existing files for equivalence" so that it reads "if the serial numbers are the same, and the high parts are the same, and the low parts are the same, then we're equivalent" avoids the confusing negation in the original form - great!). For others, I am unsure how they are improving consistency - e.g. some conditions are being reversed to return-on-success (e.g. __std_fs_get_current_path()), but others are being reversed to return-on-failure (e.g. _Open_dir()). Having a changelog would help me and other reviewers understand the changes. It doesn't need to be super detailed, it just needs to mention why each function is being changed.
   • This is also useful for "source code archaeology" - as the STL is immortal, we occasionally need to look up why things were changed 5 or 10 years ago. Having a PR description that explains each change will be helpful if we need to read it in 2027 or 2032 in our flying cars.

[StephanTLavavej]

Thanks, this looks good! I found no correctness issues and only minor stylistic issues, so I'll go ahead and push changes for them.

[strega-nil-ms]

In general, I like this; however, comparing BOOL against 0 is not good imo, and so I'm requesting that be changed.

[AZero13]

Isn't that the convention?

[StephanTLavavej]

The STL maintainers talked about BOOL on Discord - we hadn't previously settled on a convention for it, since its intended meaning is different from ordinary ints. We agreed with Nicole's preferred direction - directly testing it with if (_Ok), if (!_Ok), etc. (i.e. contextually converting it to bool). (When a real bool is needed, we want to explicitly compare against 0 instead of implicitly converting - that case is fairly rare.)

For ordinary integers, the convention is unchanged - always explicitly compare.

I'll validate and push changes, and also resolve the merge conflict here.

[StephanTLavavej]

@strega-nil-ms I pushed a merge with main, resolving a trivial merge conflict with #2802's detached comment. Then I audited filesys.cpp and filesystem.cpp for all comparisons of BOOL and BOOLEAN against 0 - both pre-existing occurrences, and ones that were introduced by this PR. (I checked every single function, instead of doing a search-and-replace, and was careful to not flip any comparisons accidentally.) I avoided changing the code that #2771 is replacing. I also reversed DeviceIoControl()'s flow to be if-success, since that's consistent with other changes here.

@AtariDreams Thanks for your patience - we should be able to merge this soon. The last thing is that we have to avoid significant changes to msvcp140.dll until VS 2022 17.4 Preview 2 is open for changes, so after Nicole double-checks the final changes here, we'll need to hold this PR in a blocked state for a little while along with a few other PRs we've accumulated.

[strega-nil-ms]

Thanks, approved!

[strega-nil-ms]

I'm still curious why this is using 0u instead of just 0; no changes requested.

[strega-nil-ms]

This is now Ready To Merge, but I'm marking it as Blocked until changes start flowing into VS 2022 17.4 Preview 2 in a couple of weeks.

[StephanTLavavej]

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

[StephanTLavavej]

Thanks for finding and improving this filesystem code! 📂 ✅ 😸