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

combine std.array.split() overloads into one #5063

Closed
wants to merge 1 commit into from

Conversation

WalterBright
Copy link
Member

@WalterBright WalterBright commented Jan 23, 2017

This replaces 2 overloads of split() with complex constraints into one function, with the variants selected via static if. A custom error message is generated for incorrect arguments.

One behavior change of this is if someone has their split(a,b) function in another import, the compiler will complain about ambiguity for which one to call, as this new version will accept anything for a and b. The previous constraints are suspect anyway, as they would accept nonsense like split("ab",1). I did not correct that issue in this PR.

@WalterBright WalterBright force-pushed the std.array.split branch 4 times, most recently from 8deb8d6 to c92c5bc Compare January 24, 2017 00:28
@WalterBright
Copy link
Member Author

Rant:

///ditto
auto split(alias isTerminator, Range)(Range range)
if (isForwardRange!Range && is(typeof(unaryFun!isTerminator(range.front))))

There is no documentation for this, and no unit tests. There are some calls for it in std.algorithm.iterator. Overloads like this are a bad practice. It should have had a different name.

@WalterBright
Copy link
Member Author

Well well, turns out there's another overload of split() in std.regex:

public @trusted String[] split(String, RegEx)(String input, const RegEx rx)

No wonder nobody can figure out what is going on in Phobos.

@WalterBright
Copy link
Member Author

So I gave up because the constraints could not be simplified, and settled for putting in a reference to the std.regex.split.

@WalterBright
Copy link
Member Author

I've noticed this on other PRs - we seem to have a heisenbug:

timelimit -t 90  generated/osx/release/32/unittest/test_runner std.socket
****** FAIL debug32 std.regex.internal.tests3
core.exception.RangeError@std/regex/package.d(567): Range violation
----------------
4   test_runner                         0x0151e499 onRangeError + 65
5   test_runner                         0x0151e66a _d_arraybounds + 22
6   test_runner                         0x013c08f7 std.regex.__array + 47
7   test_runner                         0x011940f7 std.regex.Captures!(immutable(char)[], uint).Captures.opIndex!().opIndexinout(pure nothrow @trusted inout(immutable(char)[]) function(uint)) + 543
8   test_runner                         0x014b8b9f @safe std.regex.Splitter!(0, immutable(char)[], std.regex.internal.ir.Regex!(char).Regex).Splitter std.regex.internal.tests3.__unittestL308_36().matchToRefs!(std.regex.Captures!(immutable(char)[], uint).Captures).matchToRefs(std.regex.Captures!(immutable(char)[], uint).Captures) + 71
9   test_runner                         0x014b9d6e @property @safe std.regex.Splitter!(0, immutable(char)[], std.regex.internal.ir.Regex!(char).Regex).Splitter std.algorithm.iteration.__T9MapResultS603std5regex8internal6tests317__unittestL308_36FZ11matchToRefsTS3std5regex108__T10RegexMatchTAyaS853std5regex8internal12backtracking29__T19BacktrackingMatcherVbi1Z19BacktrackingMatcherZ10RegexMatchZ.MapResult.front() + 214
10  test_runner                         0x014ba692 ref @safe std.algorithm.iteration.joiner!(std.algorithm.iteration.__T9MapResultS603std5regex8internal6tests317__unittestL308_36FZ11matchToRefsTS3std5regex108__T10RegexMatchTAyaS853std5regex8internal12backtracking29__T19BacktrackingMatcherVbi1Z19BacktrackingMatcherZ10RegexMatchZ.MapResult).joiner(std.algorithm.iteration.__T9MapResultS603std5regex8internal6tests317__unittestL308_36FZ11matchToRefsTS3std5regex108__T10RegexMatchTAyaS853std5regex8internal12backtracking29__T19BacktrackingMatcherVbi1Z19BacktrackingMatcherZ10RegexMatchZ.MapResult).Result std.algorithm.iteration.joiner!(std.algorithm.iteration.__T9MapResultS603std5regex8internal6tests317__unittestL308_36FZ11matchToRefsTS3std5regex108__T10RegexMatchTAyaS853std5regex8internal12backtracking29__T19BacktrackingMatcherVbi1Z19BacktrackingMatcherZ10RegexMatchZ.MapResult).joiner(std.algorithm.iteration.__T9MapResultS603std5regex8internal6tests317__unittestL308_36FZ11matchToRefsTS3std5regex108__T10RegexMatchTAyaS853std5regex8internal12backtracking29__T19BacktrackingMatcherVbi1Z19BacktrackingMatcherZ10RegexMatchZ.MapResult).Result.__ctor(std.algorithm.iteration.__T9MapResultS603std5regex8internal6tests317__unittestL308_36FZ11matchToRefsTS3std5regex108__T10RegexMatchTAyaS853std5regex8internal12backtracking29__T19BacktrackingMatcherVbi1Z19BacktrackingMatcherZ10RegexMatchZ.MapResult) + 266
11  test_runner                         0x014ba421 @safe std.algorithm.iteration.joiner!(std.algorithm.iteration.__T9MapResultS603std5regex8internal6tests317__unittestL308_36FZ11matchToRefsTS3std5regex108__T10RegexMatchTAyaS853std5regex8internal12backtracking29__T19BacktrackingMatcherVbi1Z19BacktrackingMatcherZ10RegexMatchZ.MapResult).joiner(std.algorithm.iteration.__T9MapResultS603std5regex8internal6tests317__unittestL308_36FZ11matchToRefsTS3std5regex108__T10RegexMatchTAyaS853std5regex8internal12backtracking29__T19BacktrackingMatcherVbi1Z19BacktrackingMatcherZ10RegexMatchZ.MapResult).Result std.algorithm.iteration.joiner!(std.algorithm.iteration.__T9MapResultS603std5regex8internal6tests317__unittestL308_36FZ11matchToRefsTS3std5regex108__T10RegexMatchTAyaS853std5regex8internal12backtracking29__T19BacktrackingMatcherVbi1Z19BacktrackingMatcherZ10RegexMatchZ.MapResult).joiner(std.algorithm.iteration.__T9MapResultS603std5regex8internal6tests317__unittestL308_36FZ11matchToRefsTS3std5regex108__T10RegexMatchTAyaS853std5regex8internal12backtracking29__T19BacktrackingMatcherVbi1Z19BacktrackingMatcherZ10RegexMatchZ.MapResult) + 153
12  test_runner                         0x014a40ef void std.regex.internal.tests3.__unittestL308_36() + 7983
13  test_runner                         0x0149ee87 void std.regex.internal.tests3.__modtest() + 175
14  test_runner                         0x0004a97e void test_runner.doTest(object.ModuleInfo*, ref bool) + 70
15  test_runner                         0x0004a8b7 bool test_runner.testModules() + 295
16  test_runner                         0x0004a784 bool test_runner.tester() + 20
17  test_runner                         0x0151f173 runModuleUnitTests + 187
18  test_runner                         0x01537515 void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).runAll() + 37
19  test_runner                         0x01537492 void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).tryExec(scope void delegate()) + 34
20  test_runner                         0x015373de _d_run_main + 686
21  test_runner                         0x0004a631 main + 37
22  test_runner                         0x0004a548 start + 52
23  ???                                 0x00000001 0x0 + 1
make[1]: *** [unittest/std/regex/internal/tests3.run] Error 1

@WalterWaldron
Copy link
Contributor

I've noticed this on other PRs - we seem to have a heisenbug:

Introduced by #5042 (comment)

There seems to be no interest in tracking & fixing "random" failure type bugs, so have fun.
Re: https://forum.dlang.org/thread/[email protected]
Re: #5004

@DmitryOlshansky
Copy link
Member

@WalterBright This is completely flawed. The whole point of split in std.regex being an overload is so that overload resolution will pick up the right function based on the argument. This allows split to be extensible. Putting all rabble in one place locks down extensibility of split, so please reconsider.

@WalterBright
Copy link
Member Author

@DmitryOlshansky I understand the concept, I designed the module system to work that way. Unfortunately, the flip side of this is making code hard to understand. There are too many overloads of split scattered around, and it is hard to see who is calling what.

It is too late to change split, as that would break existing code in clumsy ways due to the overloading. I'd like to be able to grep a name and reliably find it. Too much of Phobos has too many overloads, making it unduly difficult to figure out what is happening.

With the split in std.regex, it took me a while to figure out that extra overload that I didn't know about in another module was the cause of various problems I was having.

Overloading is a great feature, but it is something that should be used with a lot of restraint. Cross-module overloading should have even more restraint.

@DmitryOlshansky
Copy link
Member

Cross-module overloading should have even more restraint.

Completely disagree. I can trivially show it on the example of Complex type. An std defines abs for integers and floating-point in e.g. std.math. Now Complex type also has abs and naturally defines it in its module. Thanks to overloading abs now works flawlessly for both types. Any other options?

Any attempts to merge overloads together just misses this critical point of independent implementations per types involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants