Skip to content

use Win32 file APIs and widePath for file ops to support long filenames #1004

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

daveinglis
Copy link

No description provided.

@daveinglis
Copy link
Author

@swift-ci test

@daveinglis daveinglis changed the title use to Win32 file APIs and widePath for file ops to support long filenames use Win32 file APIs and widePath for file ops to support long filenames Aug 7, 2025
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

}
if (RemoveDirectoryW(wpath.data())) {
return 0;
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary } else {. Just use } and the rest of the body can follow.

@daveinglis daveinglis force-pushed the windows_long_file_fix branch from 2a26b43 to e0c6ead Compare August 8, 2025 13:54
@daveinglis
Copy link
Author

@swift-ci test

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor nits, but looks good to me.

llvm::SmallVector<wchar_t, MAX_PATH> wPath;
if (llvm::sys::path::widenPath(path, wPath))
return std::string();
CreateDirectoryW(wPath.data(), NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to log the error?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a lot of logging... Are you thinking just a fprintf maybe?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ... at least we would have something to go on if it ever failed.

@daveinglis daveinglis force-pushed the windows_long_file_fix branch from e0c6ead to 3ff8d69 Compare August 8, 2025 17:25
@daveinglis
Copy link
Author

@swift-ci test

@daveinglis daveinglis force-pushed the windows_long_file_fix branch from 3ff8d69 to c9736f5 Compare August 11, 2025 12:41
@daveinglis
Copy link
Author

@swift-ci test

@daveinglis daveinglis force-pushed the windows_long_file_fix branch from c9736f5 to bb4eef8 Compare August 11, 2025 14:07
@daveinglis
Copy link
Author

@swift-ci test

@daveinglis
Copy link
Author

@swift-ci please smoke test

llvm::SmallVector<llvm::UTF16, 20> wFileName;
llvm::convertUTF8ToUTF16String(fileName, wFileName);
return SetCurrentDirectoryW((LPCWSTR)wFileName.data());
llvm::SmallVector<wchar_t, MAX_PATH> wFileName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is MAX_PATH? Note that the true max path (with the \\?\ prefix) is 32767, not 260.

Or is that just an initial allocation size, and it grows dynamically as needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is just the initial size

#else
return ::rmdir(path);
#endif
}

int sys::stat(const char *fileName, StatStruct *buf) {
#if defined(_WIN32)
return ::_stat(fileName, buf);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@compnerd Would it be simpler to use _wstat, _wrmdir, etc., vs the Win32 functions? They support long paths too, as they are just wrappers around Win32 anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants