Implement aliasSeqOf for all iterables by John-Colvin · Pull Request #5630 · dlang/phobos

John-Colvin · 2017-07-18T22:07:00Z

Hooray for static foreach 👍

dlang-bot · 2017-07-18T22:07:01Z

Thanks for your pull request and interest in making D better, @John-Colvin! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#5630"

John-Colvin · 2017-07-18T22:09:32Z

std/meta.d

-        {
-            static assert(false, "Cannot transform range of type " ~ ArrT.stringof ~ " into a AliasSeq.");
-        }
+        static foreach (size_t i, el; iter.array)


Solutions that didn't use std.array.array seemed more complicated than they were worth (which is probably not much, thinking of how current CTFE and newCTFE work).

I'm sure @UplinkCoder would know.

don't use .array please.
instead introduce a counter variable into the parent-scope.

also.
Keep that: alias aliasSeqOf = AliasSeq!(aliasSeqOf!(range[0 .. $/2]), aliasSeqOf!(range[$/2 .. $]));
As it avoids factorial blow-up.

I'm pretty sure a counter variable isn't an option unless I take the whole struct contents in to one big mixin. array is also handing autodecoding (makes me sad).

W.r.t. the recursive template, I was afraid you'd say that. So the story with static foreach is "here's a great new feature, don't use it, it's too slow"?

ping @UplinkCoder, I'd like to understand this better, because static foreach is just so very convenient.

Don't be afraid too of using static foreach.
Entire Phobos uses it nowadays (-> #5989) and there was no measurable difference in the build time of the entire testsuite.

(Granted this doesn't say much about the extra-allocation of .array)

MetaLang

I love how much more concise and simple this code is with static foreach.

Edit: it also looks like it's consistently segfaulting on Win32. Not sure if that's related to this PR or not... may be due to some weird interaction between static foreach and the CT heap allocation.

MetaLang · 2017-07-18T23:34:33Z

std/meta.d

+if (isIterable!(typeof(iter)))
 {
-    import std.traits : isArray, isNarrowString;
+    import std.traits : isNarrowString;


Please remove the unneeded imports.

MetaLang · 2017-07-18T23:35:32Z

std/meta.d

    }
 }

+import std.traits : isIterable;


Maybe the self-important template idiom could be used here to avoid the top level import.

Let's better wait for https://github.com/dlang/DIPs/blob/master/DIPs/DIP1005.md and its result / decision.

MetaLang · 2017-07-18T23:39:15Z

std/meta.d

-        {
-            static assert(false, "Cannot transform range of type " ~ ArrT.stringof ~ " into a AliasSeq.");
-        }
+        static foreach (size_t i, el; iter.array)


I'm sure @UplinkCoder would know.

MetaLang · 2017-07-18T23:47:14Z

std/meta.d

    }
 }

+@safe unittest


It could stand to have a few more unittests testing different types of iterables.

There are already quite a few existing that cover the range types, did have anything specific in mind?

Yes. Specifically, infinite ranges. I believe isIterable will let those through, because all it checks is:

is(typeof({ foreach (elem; T.init) {} }))

Ok, infinite ranges were allowed with the previous implementation as well but obviously that's very bad. Done

JackStouffer · 2017-08-01T20:23:37Z

Do we have any numbers on the slowness of static foreach vs recursive templates?

wilzbach · 2018-03-30T19:20:56Z

Ping @John-Colvin

wilzbach · 2018-04-03T10:59:07Z

std/meta.d

+
 /**
- * Converts an input range `range` to an alias sequence.
+ * Converts any foreach-iterable (e.g. an input range) $(D range) to an alias sequence.


I assume you accidentally converted the backticks back to $(D?

Must have a been a mistake when rebasing. Also, it should have been iter anyway

JackStouffer · 2018-04-03T21:03:50Z

std/meta.d

    }
 }

+import std.traits : isIterable;


Imports go at the top of the file.

The only existing top-level import in the file was not at the top, so I tried to stay in keeping. I will move both.

MetaLang · 2018-04-04T03:41:23Z

I took the liberty of adding a Params and Returns section in the documentation, as well as correcting a small typo (any foreach-iterable -> any foreach-iterable entity).

wilzbach · 2018-04-04T03:43:27Z

I think you added whitespace with this:

Check for trailing whitespace
grep -nr '[[:blank:]]$' etc std ; test $? -eq 1
std/meta.d:1126: * 
std/meta.d:1128: *     iter = the entity to convert into an `AliasSeq`. It must be able to be able to be iterated over using 
posix.mak:547: recipe for target 'style_lint' failed

MetaLang · 2018-04-04T03:44:56Z

Hmm, yeah I did. Very annoying that the Github editor does that.

wilzbach · 2018-04-04T04:31:13Z

Looks like we are running into an ICE when building the docs - it's probably the same as recently seen in
https://issues.dlang.org/show_bug.cgi?id=18721

wilzbach · 2018-04-04T04:38:21Z

Oh and the persistent Win32 access violation means that there are now too many symbols in the OMF object file (the maximum is 32768).
So I assume another "split-up" of phobos.lib is required :/
See also: https://issues.dlang.org/show_bug.cgi?id=4904

Copyright (C) Digital Mars 2000-2007

We really need to switch to a more modern, actively maintained linker ...

John-Colvin · 2018-04-04T08:58:06Z

@MetaLang seeing as iterable is both an adjective and a noun*, I would say foreach-iterable is also both. It's ok, I'm fine with it written either way :)

* or at least it's commonly used that way and this is english after all, inventing words is fine haha

@wilzbach do you know of a workaround for either of those, or will the PR have to wait for them both to be fixed?

wilzbach · 2018-04-04T11:32:59Z

@wilzbach do you know of a workaround for either of those

Well I tried to reduce and fix the ICE, but I came out with this (dlang/dmd#8128), not really a solution :/

or will the PR have to wait for them both to be fixed?

I don't think that OMF will ever be fixed. I even asked Walter once:

That's a limitation of the OMF object file format. Keep in mind that OMF was designed for 8 bit CPUs :-)
It's not really fixable without redesigning the OMF format.
...
If you're interested, the limit is on page 2 under "Indexed References" http://www.azillionmonkeys.com/qed/Omfg.pdf

AFAICT the "workaround" against the OMF maximum symbol limit was to build the unittests separately e.g.

phobos/win32.mak

Lines 420 to 424 in 307dd15

    
           unittest : $(LIB) 
        
           	$(DMD) $(UDFLAGS) -L/co -c  -ofunittest1.obj $(SRC_STD_1) 
        
           	$(DMD) $(UDFLAGS) -L/co -c  -ofunittest2.obj $(SRC_STD_RANGE) 
        
           	$(DMD) $(UDFLAGS) -L/co -c  -ofunittest2a.obj $(SRC_STD_2a) 
        
           	$(DMD) $(UDFLAGS) -L/co -c  -ofunittest3.obj $(SRC_STD_3)

Now, I'm not sure whether this is possible, but in theory the same could be done with phobos.lib

phobos/win32.mak

Lines 397 to 398 in 307dd15

    
           $(DMD) -lib -of$(LIB) -Xfphobos.json $(DFLAGS) $(SRC_TO_COMPILE) \ 
        
           	$(ZLIB) $(DRUNTIMELIB)

So I think this could work:

	$(DMD) $(DFLAGS) -L/co -c  -oflib1.obj $(SRC_STD_1)
	$(DMD) $(DFLAGS) -L/co -c  -oflib2.obj $(SRC_STD_RANGE)
	$(DMD) $(DFLAGS) -L/co -c  -oflib3.obj $(SRC_STD_2a)
....

	$(DMD) $(DFLAGS) -L/co  unittest.d lib1.obj lib2.obj ...\
		$(ZLIB) $(DRUNTIMELIB)

(I don't use Windows either)

wilzbach · 2019-05-09T16:26:42Z

(rebased)

John-Colvin · 2019-05-11T17:02:32Z

Well this is a PR I'd completely forgotten about! Nice to see it merged.

John-Colvin requested review from MetaLang, PetarKirov and dnadlinger as code owners July 18, 2017 22:07

John-Colvin commented Jul 18, 2017

View reviewed changes

MetaLang suggested changes Jul 18, 2017

View reviewed changes

John-Colvin force-pushed the improve_aliasSeq branch from 785bcb8 to b77e021 Compare September 19, 2017 11:21

dlang-bot added Review:Needs Work Merge:stalled Merge:Needs Rebase and removed Merge:Needs Rebase labels Dec 29, 2017

John-Colvin force-pushed the improve_aliasSeq branch from b77e021 to 484c1e2 Compare April 3, 2018 10:43

dlang-bot removed Review:Needs Work Merge:stalled labels Apr 3, 2018

wilzbach reviewed Apr 3, 2018

View reviewed changes

John-Colvin force-pushed the improve_aliasSeq branch from 462a113 to 57598bd Compare April 3, 2018 20:45

JackStouffer added the Severity:Enhancement label Apr 3, 2018

JackStouffer reviewed Apr 3, 2018

View reviewed changes

MetaLang approved these changes Apr 4, 2018

View reviewed changes

wilzbach force-pushed the improve_aliasSeq branch from 6f0382a to 329cd6e Compare April 6, 2018 14:39

John-Colvin and others added 8 commits May 9, 2019 17:25

implement aliasSeqOf for all iterables

2c56956

Remove unnecessary import

f246985

fix param name in docs

9a05c1d

test for infinite ranges

c3ebee9

formatting

13fcd0c

move imports to top

0998483

Add Params and Returns sections, and fix typo

6140c01

Remove trailing whitespace

4bfcd20

wilzbach force-pushed the improve_aliasSeq branch from 329cd6e to 4bfcd20 Compare May 9, 2019 16:26

wilzbach added the Merge:auto-merge label May 9, 2019

dlang-bot merged commit b83488e into dlang:master May 9, 2019

Uh oh!

Conversation

John-Colvin commented Jul 18, 2017

Uh oh!

dlang-bot commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

John-Colvin Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MetaLang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Aug 1, 2017

Uh oh!

wilzbach commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MetaLang commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Apr 4, 2018

Uh oh!

MetaLang commented Apr 4, 2018

Uh oh!

wilzbach commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Apr 4, 2018

Uh oh!

John-Colvin commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Apr 4, 2018

Uh oh!

wilzbach commented May 9, 2019

Uh oh!

John-Colvin commented May 11, 2019

Uh oh!

dlang-bot commented Jul 18, 2017 •

edited

Loading

John-Colvin Jul 19, 2017 •

edited

Loading

MetaLang left a comment •

edited

Loading

MetaLang commented Apr 4, 2018 •

edited

Loading

wilzbach commented Apr 4, 2018 •

edited

Loading

John-Colvin commented Apr 4, 2018 •

edited

Loading