filesystem_space.cpp: Consider avoiding reinterpret_cast

[StephanTLavavej]

STL/stl/src/filesystem_space.cpp

Lines 19 to 33 in da0d8cf

	static_assert(sizeof(uintmax_t) == sizeof(ULARGE_INTEGER) && alignof(uintmax_t) == alignof(ULARGE_INTEGER),
	"Size and alignment must match for reinterpret_cast<PULARGE_INTEGER>");
	[[nodiscard]] __std_win_error _Fs_space_attempt(wchar_t* const _Temp_buffer, const DWORD _Temp_buffer_characters,
	const wchar_t* const _Target, uintmax_t* const _Available, uintmax_t* const _Total_bytes,
	uintmax_t* const _Free_bytes) noexcept {
	if (GetVolumePathNameW(_Target, _Temp_buffer, _Temp_buffer_characters)) {
	if (GetDiskFreeSpaceExW(_Temp_buffer, reinterpret_cast<PULARGE_INTEGER>(_Available),
	reinterpret_cast<PULARGE_INTEGER>(_Total_bytes), reinterpret_cast<PULARGE_INTEGER>(_Free_bytes))) {
	return __std_win_error::_Success;
	}
	}
	return __std_win_error{GetLastError()};
	}

In #356 (comment), Charles Milette (@sylveon) asked whether this static_assert and reinterpret_cast technique is necessary. I believe that we could avoid this by defining ULARGE_INTEGER local variables, passing their addresses to GetDiskFreeSpaceExW(), and then if successful, writing their QuadParts to _Available, _Total_bytes, and _Free_bytes. This would involve a few more instructions, but I don't believe that the code size and runtime impact would be observable, and avoiding reinterpret_cast may be worth it.

[BillyONeal]

I don't see any value in avoiding the reinterpret_cast.

[StephanTLavavej]

I'd argue that there's a minor benefit. reinterpret_cast is the scariest cast, so avoiding it whenever possible makes the remaining necessary occurrences stand out more, where they can be carefully analyzed for correctness.

That said, because this is a separately compiled source file calling a separately compiled API, we don't need to worry about things like strict aliasing, and I certainly don't think that there are major benefits.

I'm happy to close this as wontfix if nobody feels strongly about it.

[sylveon]

If uintmax_t ever becomes bigger in the future (like 128-bits), there could be a benefit to doing

void DoThing(uintmax_t *size)
{
	ULARGE_INTEGER u;
	GetFileSizeOrWhatever(&u);
	*size = u.QuadPart;
}

because then the source code would not require manual adjustment due to an implicit conversion happening (although, the static_assert already makes sure that such a breakage does not go silent)

[StephanTLavavej]

That's true (although changing the size of uintmax_t would be ABI-breaking, and is unlikely to happen within a decade).

[sylveon]

I can make a PR for this tomorrow if there's no objections.

[StephanTLavavej]

I think such a PR would be useful for us to compare the before and after states, although we may decide against changing anything here, depending on what the other STL maintainers think. (If the necessary change were extensive, I'd advise against doing work that might not be accepted, but this change should be quite small and easy to validate by just building.)

[BillyONeal]

I'd argue that there's a minor benefit. reinterpret_cast is the scariest cast, so avoiding it whenever possible makes the remaining necessary occurrences stand out more, where they can be carefully analyzed for correctness.

I guess I find the cast less offensive than the code that I think is going to have to be written instead (more things to get wrong, more wires to get crossed, etc). Plus the codegen will be worse (though that's secondary).

Certainly not an over my dead body thing.

we don't need to worry about things like strict aliasing

There's no strict aliasing issue here due to the common initial subsequence rule, and this is a cast from a struct to the type of its first member.

[StephanTLavavej]

After comparing the before-and-after code, a majority of the team preferred to keep the code as-is, so we're resolving this as wontfix.