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

Change AliasSeq(0, ...) uses of foreach to static foreach #5729

Merged
merged 1 commit into from
Nov 8, 2017

Conversation

wilzbach
Copy link
Member

I started to have a quick look at introducing more static foreach at Phobos.
Sadly even the naive idea to mark all foreach loops which definitely are at Compile-Time - e.g. with AliasSeq doesn't work:

sed -E "s/foreach(.*)AliasSeq/static foreach\1AliasSeq/" -i **/*.d

or in simpler words with a simplified example from Phobos (most of the uses of foreach + AliasSeq are like this):

import std.stdio, std.meta;
void main(string[] args)
{
    foreach (S; AliasSeq!(string, wstring, dstring))
    {
        auto s = S.stringof;
        writeln(s);
    }
}

https://run.dlang.io/gist/6ae32becc7ffaa1296a1a4752cebc294?compiler=dmd

And now with static foreach this fails due to the symbol being in the outer scope:

import std.stdio, std.meta;
void main(string[] args)
{
    static foreach (S; AliasSeq!(string, wstring, dstring))
    {
        auto s = S.stringof; // Error: declaration foo.main.s is already defined
        writeln(s);
    }
}

FWIW Local Declarations would solve this and FYI @tgehr mentioned that he has a working implementation of __local already.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

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.

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

LGTM, apart from some minor suggestions for std.datetime.

@@ -9858,23 +9858,17 @@ private int cmpTimeUnitsCTFE(string lhs, string rhs) @safe pure nothrow @nogc
import std.format : format;
import std.meta : AliasSeq;

Copy link
Member

@PetarKirov PetarKirov Sep 10, 2017

Choose a reason for hiding this comment

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

Changes to this module are a good candidate for a separate commit.

@@ -9858,23 +9858,17 @@ private int cmpTimeUnitsCTFE(string lhs, string rhs) @safe pure nothrow @nogc
import std.format : format;
import std.meta : AliasSeq;

static string genTest(size_t index)
static assert(timeStrings.length == 10);
Copy link
Member

Choose a reason for hiding this comment

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

This assert is not necessary. It was there in the original code only because, the author didn't use foreach (n; staticIota!(0, timeStrings.length)) (e.g. staticIota from std.typecons).

@@ -9858,23 +9858,17 @@ private int cmpTimeUnitsCTFE(string lhs, string rhs) @safe pure nothrow @nogc
import std.format : format;
import std.meta : AliasSeq;

static string genTest(size_t index)
static assert(timeStrings.length == 10);
static foreach (i; 0 .. timeStrings.length)
Copy link
Member

@PetarKirov PetarKirov Sep 10, 2017

Choose a reason for hiding this comment

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

Naming the indexes curr, next (cmp(t[curr], t[next]) == -1) and prev (cmp(t[curr], t[prev]) == 1) respectively would make the code a bit clearer.

@tgehr
Copy link
Contributor

tgehr commented Sep 10, 2017

@wilzbach: If you want to replace an existing foreach loop with static foreach, you can use two nested curly braces to introduce a new scope. (__local is useful mostly at declaration scope and for the case where some declarations should be hidden inside a single iteration while others should be visible.)

Copy link
Member

@MetaLang MetaLang left a comment

Choose a reason for hiding this comment

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

I love how much nicer static foreach makes the mixin-heavy code look.

Also looks like CircleCI doesn't know about the feature yet.

@wilzbach
Copy link
Member Author

Also looks like CircleCI doesn't know about the feature yet.

Yup -> we need to update Dscanner: #5732

@wilzbach
Copy link
Member Author

wilzbach commented Sep 14, 2017

Also looks like CircleCI doesn't know about the feature yet.

With #5732 finally being merged, the first use of static foreach in Phobos should be able to get in :)

std/math.d Outdated
@@ -8154,14 +8154,14 @@ if (isNumeric!X)
{
immutable min_sub = X.min_normal * X.epsilon;

foreach (x; AliasSeq!(smallP2, min_sub, X.min_normal, .25L, 0.5L, 1.0L,
2.0L, 8.0L, pow(2.0L, X.max_exp - 1), bigP2))
static foreach (x; [smallP2, min_sub, X.min_normal, .25L, 0.5L, 1.0L,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a static foreach to begin with?

{
assert( isPowerOf2(cast(X) x));
assert(!isPowerOf2(cast(X)-x));
}

foreach (x; AliasSeq!(0.0L, 3 * min_sub, smallP7, 0.1L, 1337.0L, bigP7, X.max, real.nan, real.infinity))
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Member

Choose a reason for hiding this comment

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

If aliasseq is not being used, can the import be removed also? (assuming that it is local import)

Copy link
Member

Choose a reason for hiding this comment

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

The import is local, but it is used for the foreach (X; AliasSeq!(float, double, real)) above.

@MartinNowak
Copy link
Member

MartinNowak commented Oct 3, 2017

The fact that static foreach doesn't introduce a new scope with the curly braces is by design.
Though my second usage also ended with this and left me a bit puzzled.
Guess it was intended to be consistent with static if, and also to slightly simplify adding top-level declarations.
Though for the latter a loop body without braces could have been another option.

static foreach (i; 0 .. 3)
    mixin("struct S"~i.to!string~" {}");

Using more than one statement is quite a pain anyhow cause you need to escape any declarations with a unique name (or use __local which seems a bit hacky).

Anyone thought about explicit exporting instead of explicit local @tgehr?

static foreach (i; 0 .. 3)
{
    immutable s = generateCode(i);
    struct S { mixin(code); }
    // just some syntax ideas
    alias scope OuterName=S;
    alias return OuterName=S;
    alias export OuterName=S;
    alias export : OuterName=S, OuterName2=S;
}

@MetaLang
Copy link
Member

MetaLang commented Oct 3, 2017

@MartinNowak is there a reason that you can't just open up another scope? e.g.:

static foreach (i; 0 .. 3)
{
    alias _ = doSomeStuff!i; //Error: multiple definitions by default
}

static foreach (i; 0 .. 3)
{{
    alias _ = doSomeStuff!i; //Okay
}}

@tgehr
Copy link
Contributor

tgehr commented Oct 4, 2017

@MartinNowak: Yes, the goal was to have the same behaviour for static foreach and static if. (I did think about having it the other way.) Being explicit about exporting does have some advantages, but apart from inconsistency with static if it also has the drawback that people need to remember how to do it even if all declarations should be visible, which is probably a common case.

Note that __local is not in the language now (exactly because the syntax looks hacky), so if you have a good proposal for syntax for that feature, we can perhaps get it into the language in a way that is satisfying to you.

However, if you want to use static foreach as just an unrolled loop at statement scope, probably it is better to use the suggestion of @MetaLang .

Another idea I had was to allow enum and alias on regular foreach loop variables, such that they force expansion:

foreach(enum i; 0 .. 3)
{
    alias _ = doSomeStuff!i; //Okay
}

This is IMHO more natural if the goal is to unroll a loop at statement scope.

@MartinNowak
Copy link
Member

Note that __local is not in the language now (exactly because the syntax looks hacky), so if you have a good proposal for syntax for that feature, we can perhaps get it into the language in a way that is satisfying to you.

Let's see what uses and hindrances for static foreach really come up, we might not even need this in the end.
Sth. like alias scope = bla; could be an alternative syntax for that __local thing.

Another idea I had was to allow enum and alias on regular foreach loop variables, such that they force expansion:

foreach(enum i; 0 .. 3)
{
alias _ = doSomeStuff!i; //Okay
}

This is IMHO more natural if the goal is to unroll a loop at statement scope.

Would be interesting, though as we can already do this with staticIota and/or AliasSeq, it's not too important. Being able to easily unroll a range w/o template recursion would be nice though.

@wilzbach
Copy link
Member Author

wilzbach commented Nov 2, 2017

Converted the static foreach in std.math to a normal foreach as suggested.

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

Successfully merging this pull request may close these issues.

8 participants