This is an archive of the discontinued LLVM Phabricator instance.

Use Windows Vista API to get the user's home directory
ClosedPublic

Authored by chfast on Oct 14 2015, 3:58 PM.

Details

Summary

This patch replaces usage of deprecated SHGetFolderPathW with SHGetKnownFolderPath. The usage of SHGetKnownFolderPath is wrapped to allow queries for other "known" folders in the near future.

Diff Detail

Repository
rL LLVM

Event Timeline

chfast updated this revision to Diff 37412.Oct 14 2015, 3:58 PM
chfast retitled this revision from to Use Windows Vista API to get the user's home directory.
chfast updated this object.
chfast set the repository for this revision to rL LLVM.
chfast added a subscriber: llvm-commits.
gbedwell added inline comments.Oct 15 2015, 4:49 AM
lib/Support/Windows/WindowsSupport.h
29–30

Please can you split this part off into its own change?

I've had something similar on my TODO for far too long (sorry!), so I'm very happy to see this being done. Given the previous discussion on Windows versions, I think we should jump straight to Windows 7 API here (we've already said we're doing that in the release notes), and ideally we might accompany this by a check/warning in CMake as a courtesy although I don't feel too strongly about it. I think a check based on ${CMAKE_SYSTEM_VERSION} should work.

Sure, I will do it.

CMake warning should be about "you're building LLVM on a version of Windows that not meet minimum requirements"?
I think the main problem is *running* binaries (built on supported Windows) on not-supported versions of Windows, isn't it?

gbedwell edited edge metadata.Oct 15 2015, 5:31 AM

CMake warning should be about "you're building LLVM on a version of Windows that not meet minimum requirements"?
I think the main problem is *running* binaries (built on supported Windows) on not-supported versions of Windows, isn't it?

This should be the distinction between CMAKE_HOST_SYSTEM_VERSION (OS version CMake is running on) and CMAKE_SYSTEM_VERSION (The OS version CMake is building for) (according to https://cmake.org/cmake/help/v2.8.12/cmake.html#section_VariablesThatDescribetheSystem ). My fear that it was still possible to build with the CMake option "-T vs120_xp" to specify an XP targeting platform toolset, but having just tried it to see whether it affects CMAKE_SYSTEM_VERSION or not, I've not actually been able to get that far without running into weird CMake errors so I'm not convinced that anyone would be able to use it anyway. As I said, I don't feel too strongly about this. It's been documented for a while that XP will no longer be supported and nobody leapt into the discussion to state that they still cared, so I don't think it's important to present a user friendly CMake-time error. It would just be nice if it turned out to be trivial.

aaron.ballman accepted this revision.Oct 15 2015, 5:50 AM
aaron.ballman edited edge metadata.

LGTM, once you split out the switch to Win 7 support.

lib/Support/Windows/WindowsSupport.h
29–30

I agree that this should be changd to require at least Windows 7, and as a separate commit. You may also want to see whether we need to change _WIN32_IE as well while we're at it.

This revision is now accepted and ready to land.Oct 15 2015, 5:50 AM

My fear that it was still possible to build with the CMake option "-T vs120_xp" to specify an XP targeting platform toolset, but having just tried it to see whether it affects CMAKE_SYSTEM_VERSION or not, I've not actually been able to get that far without running into weird CMake errors so I'm not convinced that anyone would be able to use it anyway. As I said, I don't feel too strongly about this. It's been documented for a while that XP will no longer be supported and nobody leapt into the discussion to state that they still cared, so I don't think it's important to present a user friendly CMake-time error. It would just be nice if it turned out to be trivial.

I've checked that. -T "v120_xp" option do not modify CMAKE_SYSTEM variables. The toolset option itself can by queried with CMAKE_GENERATOR_TOOLSET variable but only if was explicitly set. Considering all possible combination of options (MSVC, toolset, mingw32, mingw-w64) I would avoid detecting unsupported targeted Windows version in cmake script.

I've checked that. -T "v120_xp" option do not modify CMAKE_SYSTEM variables. The toolset option itself can by queried with CMAKE_GENERATOR_TOOLSET variable but only if was explicitly set. Considering all possible combination of options (MSVC, toolset, mingw32, mingw-w64) I would avoid detecting unsupported targeted Windows version in cmake script.

Great. Thanks for checking! I absolutely agree with your reasoning, so please do not worry about the CMake check suggestion.

chfast added inline comments.Oct 15 2015, 6:39 AM
lib/Support/Windows/WindowsSupport.h
29–30

_WIN32_IE looks to be needed because mingw fails to define it basic on _WIN32_WINNT? I'm not able to verify that (happily haven't been using mingw for years). I will update it to the value matching Windows 7.

chfast updated this revision to Diff 37524.Oct 15 2015, 2:56 PM
chfast updated this object.
chfast edited edge metadata.

Required Windows API bump committed in as separated change http://reviews.llvm.org/rL250413.

This update also modifies the unit test to handle path containing non-ASCII characters.

This revision was automatically updated to reflect the committed changes.