Skip to content
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

Support for Windows Long Paths #398

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ashishk98
Copy link

Add support for windows long paths


private:
inline std::string getOSValidPath(const std::string& _path);
const char* kWindowsLongPathPrefix = "\\\\?\\";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const char* kWindowsLongPathPrefix = "\\\\?\\";
const char* windows_long_path_prefix_ = "\\\\?\\";

To match Triton member variable convention.

Choose a reason for hiding this comment

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

But that's a constant, not a variable? AFAIK, constants should be named like the former, not the latter.

Choose a reason for hiding this comment

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

It might be more idiomatic to use static constexpr char* kWindowsLongPathPrefix = "\\\\?\\"; to drive the fact this is, indeed, a constant.

Copy link
Contributor

@fpetrini15 fpetrini15 Oct 18, 2024

Choose a reason for hiding this comment

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

Had to verify with the google style guide and you're both correct. I also found an instance in Triton code where we initialize to a constant here, however, we omit the leading "k", which is technically wrong according to the style standards.

We can defer to Nico's suggestion instead.

@@ -60,12 +60,45 @@ class LocalFileSystem : public FileSystem {
Status MakeTemporaryDirectory(
std::string dir_path, std::string* temp_dir) override;
Status DeletePath(const std::string& path) override;

private:
inline std::string getOSValidPath(const std::string& _path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
inline std::string getOSValidPath(const std::string& _path);
inline std::string GetOSValidPath(const std::string& path);

To match triton syntax conventions

//! https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry
//!
inline std::string
LocalFileSystem::getOSValidPath(const std::string& _path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
LocalFileSystem::getOSValidPath(const std::string& _path)
LocalFileSystem::getOSValidPath(const std::string& path)

Cascading effect where the local path variable will need to change to something like local_path or l_path

@fpetrini15
Copy link
Contributor

fpetrini15 commented Oct 18, 2024

Great work @ashishk98, just a few minor stylistic comments to be consistent with Triton. Also, to get this merged, I need two things:

  1. Can you port these changes to be a direct branch from main -- our CI is not built around cloning forked branches
  2. Can you update the copyright at the top of the file to extend to 2024?

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

Successfully merging this pull request may close these issues.

4 participants