Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1458)

Issue 5712045: code review 5712045: path/filepath: steer people away from HasPrefix (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 9 months ago by rsc
Modified:
9 years, 9 months ago
Reviewers:
CC:
golang-dev, r, dsymonds, r2
Visibility:
Public.

Description

path/filepath: steer people away from HasPrefix The strikes against it are: 1. It does not take path boundaries into account. 2. It assumes that Windows==case-insensitive file system and non-Windows==case-sensitive file system, neither of which is always true. 3. Comparing ToLower against ToLower is not a correct implementation of a case-insensitive string comparison. 4. If it returns true on Windows you still don't know how long the matching prefix is in bytes, so you can't compute what the suffix is.

Patch Set 1 #

Patch Set 2 : diff -r b1f0c3b7a935 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r b1f0c3b7a935 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 37763a8d9c71 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -4 lines) Patch
M src/pkg/path/filepath/path_plan9.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/path/filepath/path_unix.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/path/filepath/path_windows.go View 1 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 6
rsc
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
9 years, 9 months ago (2012-02-29 21:04:53 UTC) #1
rsc
I updated the CL description with the rationale: path/filepath: steer people away from HasPrefix The ...
9 years, 9 months ago (2012-02-29 21:06:12 UTC) #2
r
LGTM
9 years, 9 months ago (2012-02-29 21:14:21 UTC) #3
dsymonds
What historical compatibility do we care about enough to preserve this? Could we drop it ...
9 years, 9 months ago (2012-02-29 21:20:16 UTC) #4
r2
On 01/03/2012, at 8:20 AM, David Symonds wrote: > What historical compatibility do we care ...
9 years, 9 months ago (2012-02-29 21:22:47 UTC) #5
rsc
9 years, 9 months ago (2012-02-29 21:37:45 UTC) #6
*** Submitted as http://code.google.com/p/go/source/detail?r=aa7ded1d6fab ***

path/filepath: steer people away from HasPrefix

The strikes against it are:

1. It does not take path boundaries into account.
2. It assumes that Windows==case-insensitive file system
and non-Windows==case-sensitive file system, neither of
which is always true.
3. Comparing ToLower against ToLower is not a correct
implementation of a case-insensitive string comparison.
4. If it returns true on Windows you still don't know how long
the matching prefix is in bytes, so you can't compute what
the suffix is.

R=golang-dev, r, dsymonds, r
CC=golang-dev
http://codereview.appspot.com/5712045
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b