fix binaryFun #2723
fix binaryFun #2723
Conversation
|
Very nice! LGTM. I'll let other reviewers take a look as well, since this is a non-trivial change. |
| @@ -19,9 +19,11 @@ Distributed under the Boost Software License, Version 1.0. | |||
| http://www.boost.org/LICENSE_1_0.txt) | |||
| */ | |||
| module std.functional; | |||
| //debug = std_functional; | |||
There was a problem hiding this comment.
I have rather bad experience with debug(XXX) versioned blocks in published code - it tends to be both too opinionated and inflexible to be widely useful and bitrots because ignored by tester.
There was a problem hiding this comment.
This only for manual tests and debuging. See pragmas.
There was a problem hiding this comment.
I understand but it pollutes code of the module with pragmas needed primarily by you. In my opinion it is better to keep such changes in forked branch and merge on top of upstream when debugging the module. This is just my personal opinion of course but we had a lot of trouble with debug printfs / pragmas in internal Sociomantic projects exactly because of that :(
There was a problem hiding this comment.
I thought this is Phobos conversion std_modulename. See std.algorithm source code.
There was a problem hiding this comment.
Yeah I think we should keep debug statements out of Phobos code, unless it is universally useful to all Phobos developers. You can of course use them while developing your PR, but I'd propose removing them before merging into master.
There was a problem hiding this comment.
Should I remove pragmas with debug?
|
Rebased. |
|
Awesome. |
|
LGTM |
|
Does it still make sense to canonicalize the functions to once whitespace variant if there's a match? |
| if (op.length >= name.length && op[0..name.length] == name) | ||
| { | ||
| op = op[name.length..$]; | ||
| return true; |
There was a problem hiding this comment.
IMO just using 0/1 would be clearer here.
|
@klickverbot Please give me an example. I don't understand you about canonicalisation. |
|
I think he meant you should return 0 or 1 instead of |
1. Make binaryFun correct for integer comparison. 2. Remove import of half of phobos for 100% cases in phobos. `sort`, `cmp`, `find`, `startsWith`, `sum` and other algorithms would be compiled faster. fix safeOp fix safeOPs Fix new unittest fix binaryReverseArgs fix not fix new code fix spaces after if optimization 1. CTFE 2. Runtime (without inlining) change condition fix new code fix safeOp optimize CTFE CTFE optimization immutable -> enum remove aliases remove macros CTFE matching add debug info && fix not add spaces minor fix added debug example add debug unittests & fix fix also and add unittests fix typo if (!__ctfe) assert(false); added use 01
|
|
|
I meant only actually emitting one function for |
|
I think this ( emitting one function ) should be other PR. Also I don't see the fast implementation with UTF8 support and without encoding/decoding. |
|
I think we should merge this first, and collapsing functions for |
|
Auto-merge toggled on |
|
This pull request introduced a regression: |
Old log:#2720
Remove import of half of phobos for 100% cases in phobos.
sort,cmp,find,startsWith,sumand other algorithms would be compiled faster.See also: Issue 13253 - use more scoped imports in phobos