Skip to content

Use static foreach in Phobos#5989

Merged
dlang-bot merged 1 commit intodlang:masterfrom
wilzbach:static-foreach
Jan 3, 2018
Merged

Use static foreach in Phobos#5989
dlang-bot merged 1 commit intodlang:masterfrom
wilzbach:static-foreach

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Jan 3, 2018

All the cool kids in town use static foreach. Let's join the club.
Motivation: static foreach makes it clear to the reader that the loop is unrolled
during compile-time.
The replacements were done semi-automatically.

I also removed the workarounds against BUG 2396 - static foreach doesn't seem
to be affected by this as overall execution time of the entire Phobos testsuite
doesn't with this PR.

I know that this got quite large, but the changes itself are trivial.

@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

@andralex andralex left a comment

Choose a reason for hiding this comment

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

noice

foreach (endianness; AliasSeq!(Endian.bigEndian, Endian.littleEndian))
{
import std.meta : AliasSeq;
static foreach (endianness; [Endian.bigEndian, Endian.littleEndian])
Copy link
Member

Choose a reason for hiding this comment

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

Why the change from AliasSeq to an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because static foreach can unroll arrays and thus there's no need for AliasSeq anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Yes but why only this one specifically? I know most of these loops are over lists of types, but there's at least one other one that loops over a list of values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I overlooked the other case. This PR catches only 95% of the static foreach's.
Anyways, we can easily make more improvements to this. Where did you see the other list of values?

Copy link
Member

Choose a reason for hiding this comment

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

It's not that important; I was just trying to understand why you did it for one and not the others. Looking at the other cases, however, an array can't or shouldn't be used because they're separate types.

@wilzbach
Copy link
Contributor Author

wilzbach commented Jan 3, 2018

BTW seeing how easy it is to replace foreach with static foreach, should we deprecate compile-time loop unrolling in the normal foreach?
If that's not wanted, we could at least think about introducing a DScanner check.

@jmdavis
Copy link
Member

jmdavis commented Jan 3, 2018

Is there any point to using static on the foreaches which use AliasSeq other than style?

@MetaLang
Copy link
Member

MetaLang commented Jan 3, 2018

Does anyone know if dfix is still under active development? This type of simple code rewriting is exactly what it was made for.

@jmdavis
Copy link
Member

jmdavis commented Jan 3, 2018

BTW seeing how easy it is to replace foreach with static foreach, should we deprecate compile-time loop unrolling in the normal foreach?

There's a lot of code out there doing stuff like using AliasSeq with foreach. Unless we have a really good reason for deprecating the behavior, I suggest that we leave it alone. Also, I don't particularly want to restrict the compiler's ability to optimize foreach, and if trying to shift some of its current semantics over to static foreach does that, then that seems like a negative (I don't know if it would or not).

@wilzbach
Copy link
Contributor Author

wilzbach commented Jan 3, 2018

Does anyone know if dfix is still under active development? This type of code rewriting is exactly what it was made for.

AFAICT only @UplinkCoder and Sociomantic have done a bit of work on it recently.
I started https://github.com/wilzbach/dscanner-fix with the idea of automatically fixing DSCanner warnings a while ago, but it's a more or less a collection of crude hacks (though some of them have been used to upgrade Phobos).

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.

6 participants