Skip to content

gh-117352: Handle /:/ for ntpath.isabs - #117362

Closed
nineteendo wants to merge 6 commits into
python:mainfrom
nineteendo:fix-ntpath.isabs
Closed

gh-117352: Handle /:/ for ntpath.isabs#117362
nineteendo wants to merge 6 commits into
python:mainfrom
nineteendo:fix-ntpath.isabs

Conversation

@nineteendo

@nineteendonineteendo commented Mar 29, 2024

Copy link
Copy Markdown
Contributor

Benchmark:

# test.sh
python -m timeit -s "import before.ntpath""before.ntpath.isabs('C:/foo')"
python -m timeit -s "import after.ntpath""after.ntpath.isabs('C:/foo')"
500000 loops, best of 5: 398 nsec per loop # before
500000 loops, best of 5: 469 nsec per loop # after
# -> 1.18x slower

@bedevere-appbedevere-appBot mentioned this pull request Mar 29, 2024
@nineteendonineteendo mentioned this pull request Mar 29, 2024
16 tasks
@nineteendo
nineteendo marked this pull request as ready for review March 29, 2024 07:43
Comment threadLib/ntpath.py Outdated
@eryksun
eryksun requested review from barneygale and zoobaMarch 29, 2024 12:03
Co-Authored-By: Barney Gale <barney.gale@gmail.com>
@nineteendonineteendo changed the title gh-117352: Handle /: for ntpath.isabsgh-117352: Handle /:/ for ntpath.isabsMar 31, 2024
@nineteendo

Copy link
Copy Markdown
ContributorAuthor

I've reverted the slicing. The speed decrease of str.startswith() should be fixed soon.

@zooba

zooba commented Apr 2, 2024

Copy link
Copy Markdown
Member

Why would we handle /: differently from any other invalid drive character? Is there some important reason to do this as opposed to more thoroughly checking for all invalid paths?

@nineteendo

Copy link
Copy Markdown
ContributorAuthor

#101363 (comment):

WinAPI GetFullPathNameW() will accept any character in the 16-bit BMP as a drive 'letter', except for null, slash, and backslash.

You could restrict it further, but that would make the code more complicated.

@nineteendo
nineteendo deleted the fix-ntpath.isabs branch April 5, 2024 10:27
Sign up for freeto 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.

3 participants

@nineteendo@zooba@eryksun