Conversation
lygstate
force-pushed
the
normalize_fixed
branch
from
f58338e to
88e95f2
Compare
Use GetFullPathNameW() instead of GetFullPathNameA() to ensure that normalization works for all long paths on Windows. Adjust unit tests accordingly. Fixes ninja-build#2442
lygstate
force-pushed
the
normalize_fixed
branch
from
88e95f2 to
dfe1d55
Compare
|
cc @jhasse |
|
|
||
| // NOTE: wide_full_size includes the null-terminating character. | ||
| std::wstring wide_path; | ||
| wide_path.resize(static_cast<size_t>(wide_full_size - 1)); |
There was a problem hiding this comment.
I think -1 can be omitted, it's not bad to include extra NUL at the end.
here is no need to use static_cast for widen unsigned conversion (DWORD/unsigned long => size_t).
The document for GetFullPathNameW doesn't say [out] LPWSTR lpBuffer parameter can be NULL, needs test to confirm.
https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfullpathnamew
Edit: the function will check [in] DWORD nBufferLength parameter (zero in the call) first.
There was a problem hiding this comment.
You mis-understading the size parameter of std::wstring::resize, the std::wstring always have NUL at the end. but the size parameter would not count it.
There was a problem hiding this comment.
Yes, but it does not harm to have extra NUL at string end, wide_path.resize(wide_full_size) is simple.
I come up with an optimization idea, but not sure whether worth it.
- init
std::wstring wide_path(MAX_PATH, 0);,MAX_PATHshould works most of time, then do firstwide_full_size = GetFullPathNameW()call. - if result
wide_full_sizelarge thanMAX_PATH, resizewide_path.resize(wide_full_size), then do secondwide_full_size = GetFullPathNameW()call. - if
wide_full_sizeis zero, store error message.
There was a problem hiding this comment.
There is a benchmark of this functionality at clparser_perftest.exe that you can use.
Also FYI, I have a PR to optimize the performance of IncludesNormalize that will probably conflict with this PR. If this lands first I don't mind rebasing mine and looking at the performance of it.
| wide_path.resize(static_cast<size_t>(wide_full_size - 1)); | ||
| DWORD wide_full_size2 = | ||
| GetFullPathNameW(wide_filename.c_str(), wide_full_size, | ||
| const_cast<wchar_t*>(wide_path.data()), NULL); |
There was a problem hiding this comment.
I think const_cast<wchar_t*> is not need.
4 participants
Replace #2552