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

Fix LWG-3699 lexically_relative on UNC drive paths (\\?\C:\...) results in a default-constructed value #2867

Merged
merged 4 commits into from
Jul 28, 2022

Conversation

strega-nil-ms
Copy link
Contributor

@strega-nil-ms strega-nil-ms commented Jul 15, 2022

Fixes #2256 and LWG-3699

This is one choice of how to implement this; it makes lexically_relative treat \\?\<drive letter>:\ as equivalent to <drive letter>:\; another option would just be to allow \\?\<drive letter>:\ and not have them be equivalent.

@strega-nil-ms strega-nil-ms added the decision needed We need to choose something before working on this label Jul 15, 2022
@strega-nil-ms
Copy link
Contributor Author

strega-nil-ms commented Jul 15, 2022

Marking decision needed as we need to decide whether \\?\C: and C: are "the same" for purposes of lexically_relative

link to discussion with @BillyONeal

@StephanTLavavej StephanTLavavej added bug Something isn't working filesystem C++17 filesystem labels Jul 17, 2022
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good to me - all I noticed were very minor issues/questions.

tests/std/tests/P0218R1_filesystem/test.cpp Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Show resolved Hide resolved
@strega-nil-ms
Copy link
Contributor Author

@StephanTLavavej fixed :)

@strega-nil-ms strega-nil-ms marked this pull request as ready for review July 20, 2022 17:32
@strega-nil-ms strega-nil-ms requested a review from a team as a code owner July 20, 2022 17:32
@StephanTLavavej StephanTLavavej removed the decision needed We need to choose something before working on this label Jul 20, 2022
@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting and decided that the strategy of "UNC paths can be lexically relative to one another, but not UNC and DOS" is reasonable given the Standard's wording and our behavior here (where we consider C: and c: to be different for the purposes of lexically relative).

@strega-nil-ms strega-nil-ms removed their assignment Jul 20, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jul 27, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit b4c409c into microsoft:main Jul 28, 2022
@StephanTLavavej
Copy link
Member

Thanks for improving filesystem behavior on Windows! 🗄️ 🗂️ 💾

@strega-nil strega-nil deleted the fix-lexically_relative branch August 10, 2022 20:35
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
…esults in a default-constructed value (microsoft#2867)

Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
@ZubairRafiq
Copy link

ZubairRafiq commented Jun 17, 2023

`#include` <filesystem> #include <vector)
#include <string> #include <iostream>

struct Result {
      std::string s1;
      std::string s2;
      }
Result test () {
      std::string test = {R" (\\?\C:\temp\test-file.txt)");
      std::filesystem::path const p(test);
      
      Result res;
      res.s1.append(p.root_name ().string()); 
      res.s2.append(p.root_path().string());

      std:: cout <‹ std::endl <‹ test <‹ std::endl;
      std::cout <‹ "root_name\t" <‹ p.root_name ().string() <‹ std:: endl;
      std::wcout <‹ L" absolute: " <‹ std::filesystem::absolute (p).wstring() <‹ std::endl; 
      std::cout <‹ "root_directory\t" <‹ p.root_directory().string() <‹ std: :end1; 
      std::cout <‹ "root_path\t" <‹ p.root_path().string() <‹ std::endl;
      std::cout <‹ "relative_path\t" <‹ p.relative_path ().string() <‹ std:: end1; 
      std::cout <‹ "parent_path\t" <‹ p.parent_path().string() ‹‹ std::endl;
      return res;
}

The output:
root name
absolute: w:?\C: \temp\test-file.txt
root directory \
root_path \
relative_path ?\C:\temp\test-file.txt
parent_path ?\C:\temp

The “root name” component extracted from the path is reported as an empty string (””). However, the expected value was “C:”, which represents the drive letter of the path.

The root directory output is "\", however it should be "C:\".
What is the reason for this behavior of std::filesystem::path?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working filesystem C++17 filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<filesystem>: lexically_relative() can not handle long path with prefix \\?\
4 participants
  翻译: