Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Range-ify std.path.filenameCmp() #3266

Merged
merged 1 commit into from
May 22, 2015

Conversation

WalterBright
Copy link
Member

100% unit test coverage

The behavior changes here:

  1. decode only if case insensitive or the arguments are different string encodings
  2. if decoding, treat invalid UTF encodings as if they were replacementDchar's, rather than throwing an exception

This has the effect of making the function:

  1. faster
  2. nothrow and @nogc

Being a low level function, of course this affects everything up the call stack.

@quickfur
Copy link
Member

Wouldn't this cause odd behaviour if given two strings with invalid encodings in the same positions, thus causing unequal strings to compare equal?

@WalterBright
Copy link
Member Author

There are 3 options with invalid encodings:

  1. throw exception
  2. terminate program with some sort of error or assert
  3. replace with replacementDchar

Each has advantages and disadvantages. There is no correct solution other than validating your data beforehand. Consider also that floating point NaNs are regarded as equivalent even though they can have different bit patters.

I tend to favor the one with the lowest runtime cost, which is (3). It is far more practical for the programmer to add extra checks if he wants (1) or (2) than it is if he wants to remove them. In fact, to remove them he'd have to reimplement these routines.

const c = filenameCharCmp!cs(filename1.front, filename2.front);
if (c != 0) return c;
filename1.popFront();
filename2.popFront();
Copy link
Member

Choose a reason for hiding this comment

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

these two popFronts should go in the for imho

@andralex
Copy link
Member

lgtm modulo the unnecessary assert(0)

@andralex
Copy link
Member

Auto-merge toggled on

andralex added a commit that referenced this pull request May 22, 2015
@andralex andralex merged commit d1af429 into dlang:master May 22, 2015
@WalterBright WalterBright deleted the rangeFilenameCmp branch May 22, 2015 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants