path: fix win32.isAbsolute()#6028
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mscdex added windows 
mscdex commented Apr 3, 2016
benjamingr commented Apr 3, 2016
Here are some relevant unit tests for checking if a path is absolute in win32 We should port some/all of them |
| } | ||
| } | ||
| }elseif(code===47/*/*/||code===92/*\*/){ | ||
| if(code===47/*/*/||code===92/*\*/){ |
There was a problem hiding this comment.
This code is much simpler than the old code - any idea why it was so long and what purpose the checks served?
There was a problem hiding this comment.
I don't recall offhand, probably just a misinterpretation of the old regexp.
There was a problem hiding this comment.
Ok, the new version is correct on its own - the old one is just weird.
mscdex commented Apr 3, 2016
@benjamingr We already cover those more or less, especially so with the test case additions provided by this PR. |
benjamingr commented Apr 3, 2016
mscdex commented Apr 3, 2016
@benjamingr I don't recall if it was performance reasons or what. If someone wants to try factoring that out and checking the performance differences, go for it :-). However I suspect that a helper function containing that factored out portion wouldn't be inlined (if for no reason other than code size), which would cause a performance regression to some degree. |
jasnell commented Apr 4, 2016
@mscdex ... can you add a bit more explanation to the commit log? Otherwise LGTM |
This commit fixes an inconsistency in absolute path checking compared to the absolute path detection used by the other path.win32 functions. Fixes: nodejs#6027 PR-URL: nodejs#6028
mscdexforce-pushed the path-fix-win32-isabsolute branch from d35449c to 45a960eComparemscdex commented Apr 4, 2016
@jasnell Updated |
jasnell commented Apr 4, 2016
Thanks! LGTM! |
This commit fixes an inconsistency in absolute path checking compared to the absolute path detection used by the other path.win32 functions. Fixes: #6027 PR-URL: #6028 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
jasnell commented Apr 4, 2016
Landed in 3072546 |
MylesBorins commented Apr 4, 2016
mscdex commented Apr 4, 2016
@thealphanerd AFAIK backporting the tests should be fine |
jasnell commented Apr 4, 2016
+1 to backporting the tests |
Adds test cases to ensure win32.isAbsolute is consistent. This is a backport from 3072546 ref: nodejs#6028
This commit fixes an inconsistency in absolute path checking compared to the absolute path detection used by the other path.win32 functions. Fixes: #6027 PR-URL: #6028 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
Notable changes: http: * Enclose IPv6 Host header in square brackets. This will enable proper seperation of the host adress from any port reference (Mihai Potra) #5314 path: * Make win32.isAbsolute more consistent (Brian White) #6028 PR-URL: #6060 Reviewed-By: Jeremiah Senkpiel <[email protected]>
Notable changes: http: * Enclose IPv6 Host header in square brackets. This will enable proper seperation of the host adress from any port reference (Mihai Potra) #5314 path: * Make win32.isAbsolute more consistent (Brian White) #6028 PR-URL: #6060 Reviewed-By: Jeremiah Senkpiel <[email protected]>
Adds test cases to ensure win32.isAbsolute is consistent. This is a backport from 3072546 Refs: #6028 PR-URL: #6043 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
4 participants
Pull Request check-list
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
Affected core subsystem(s)
Description of change
Fixes: #6027