Skip to content

fix issue 19266 - Some source files names are no longer accepted#8741

Merged
Geod24 merged 1 commit intodlang:stablefrom
rainers:issue_19266
Sep 27, 2018
Merged

fix issue 19266 - Some source files names are no longer accepted#8741
Geod24 merged 1 commit intodlang:stablefrom
rainers:issue_19266

Conversation

@rainers
Copy link
Member

@rainers rainers commented Sep 26, 2018

only add the \?\ prefix if the filename is actually long and does not start with \ already.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @rainers!

Bugzilla references

Auto-close Bugzilla Severity Description
19266 regression Some source files names are no longer accepted

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + dmd#8741"

@Geod24
Copy link
Member

Geod24 commented Sep 26, 2018

Do you have the PR that introduced this ?

Copy link
Contributor

@jacob-carlborg jacob-carlborg left a comment

Choose a reason for hiding this comment

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

Should also have a test if possible.

static immutable prefix = `\\?\`w;

// prefix only needed for long names and non-UNC names
bool needsPrefix = pathLength >= MAX_PATH && (wpath[0] != '\\' || wpath[1] != '\\');
Copy link
Contributor

Choose a reason for hiding this comment

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

can be const.


// prefix only needed for long names and non-UNC names
bool needsPrefix = pathLength >= MAX_PATH && (wpath[0] != '\\' || wpath[1] != '\\');
size_t prefixlen = needsPrefix ? prefix.length : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

  • const
  • I prefer to not use abbreviations and to use camel case names

@rainers
Copy link
Member Author

rainers commented Sep 26, 2018

Do you have the PR that introduced this ?

#7299

@rainers
Copy link
Member Author

rainers commented Sep 26, 2018

Should also have a test if possible.

Not so easy as it requires either a network share or an absolute path to be passed with \\?\, but I'll try to add a shell script that does the latter.

@rainers
Copy link
Member Author

rainers commented Sep 26, 2018

Added test and changed code according to comments.

@@ -0,0 +1,3 @@
module test19266;

pragma(msg, __FILE__);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this doing anything for the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but you can look it up in the test_result folder. If \\?\ is given on the command line it is also found in __FILE__.

only add the \\?\ prefix if the filename is actually long and does not start with \\ already.
@Geod24
Copy link
Member

Geod24 commented Sep 27, 2018

The AppVeyor failure is due to stdint and unrelated to this. #8729 will fix it.
I checked that the added tests pass, so going to merge this manually.

@Geod24 Geod24 merged commit ac9272c into dlang:stable Sep 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments