fix issue #8251 ospaths.isAbsolute: out of bound errors #8291
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Araq
reviewed
Jul 12, 2018
lib/pure/ospaths.nim
Outdated
| (len > 1 and path[0] in {'a'..'z', 'A'..'Z'} and path[1] == ':') | ||
| elif defined(macos): | ||
| result = path.len > 0 and path[0] != ':' | ||
| result = path[0] != ':' |
There was a problem hiding this comment.
This needs to be path.len == 0 or path[0] != ':', maybe.
There was a problem hiding this comment.
- actually
oris not the right thing since empty path should not be absolute - however I'm already checking for
len=0at function top-level just found another preexisting bug which my latest commit fixes: the condition here should be negated! ":" would be absolute on macos (given that ":" meant the new "/" separator);=> actually upon reading more I found this https://perldoc.perl.org/File/Spec/Mac.html and added that as reference for this non-intuitive behavior; on that note, see my post https://forum.nim-lang.org/t/4039 "should we keep supporting macos? (as opposed to macosx)"
Araq
reviewed
Jul 12, 2018
lib/pure/ospaths.nim
Outdated
| @@ -423,12 +423,22 @@ proc isAbsolute*(path: string): bool {.rtl, noSideEffect, extern: "nos$1".} = | |||
| ## Checks whether a given `path` is absolute. | |||
| ## | |||
| ## On Windows, network paths are considered absolute too. | |||
| runnableExamples: | |||
| doAssert "".isAbsolute.not | |||
| doAssert nil.isAbsolute.not | |||
There was a problem hiding this comment.
There soon will be no nil for strings, don't write tests covering it.
Araq
reviewed
Jul 12, 2018
| when doslikeFileSystem: | ||
| var len = len(path) | ||
| result = (len > 0 and path[0] in {'/', '\\'}) or |
There was a problem hiding this comment.
I fail to see how removing bounds checking helps with anything.
There was a problem hiding this comment.
the case len=0 is already handled at the very top-level of this function
Araq
reviewed
Jul 12, 2018
lib/pure/ospaths.nim
Outdated
| @@ -423,12 +423,22 @@ proc isAbsolute*(path: string): bool {.rtl, noSideEffect, extern: "nos$1".} = | |||
| ## Checks whether a given `path` is absolute. | |||
| ## | |||
| ## On Windows, network paths are considered absolute too. | |||
| runnableExamples: | |||
| doAssert "".isAbsolute.not | |||
There was a problem hiding this comment.
.not is not a style we use anywhere else.
There was a problem hiding this comment.
that was a workaround after hitting that bug: #8300 ; but I've fixed it to doAssert(not condition)
|
PTAL |
timotheecour
force-pushed
the
ps_isAbsolute
branch
2 times, most recently
from
985f05d to
7c273f3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.