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

Add support for `static foreach` #304

Merged
merged 4 commits into from Nov 29, 2017

Conversation

@kotet
Copy link
Contributor

commented Nov 25, 2017

Fix #303

Fix #303
Copy link
Member

left a comment

This requires an update to libdparse which should probably be done separately, but I don't know if the current state is usable/stable (ref @Hackerpilot @wilzbach )

@@ -197,7 +197,8 @@ private:
immutable currentIsNonWrapTemp = !isWrapIndent(arr[i])
&& isTempIndent(arr[i]) && arr[i] != tok!")" && arr[i] != tok!"!";
if (arr[i] == tok!"static" && (arr[i + 1] == tok!"if"
|| arr[i + 1] == tok!"else") && (i + 2 >= index || arr[i + 2] != tok!"{"))
|| arr[i + 1] == tok!"else" || arr[i + 1] == tok!"foreach"
|| arr[i + 1] == tok!"foreach_reverse") && (i + 2 >= index || arr[i + 2] != tok!"{"))

This comment has been minimized.

Copy link
@WebFreak001

WebFreak001 Nov 25, 2017

Member

I would import std.algorithm : among and make this

arr[i + 1].among!(tok!"if", tok!"else", tok!"foreach", tok!"foreach_reverse")

because it has a lot of cases now

static foreach (thing; things){doStuff();}
static foreach_reverse (thing; things){doStuff();}
static foreach (thing; things) doStuff();
static foreach_reverse (thing; things) doStuff();

This comment has been minimized.

Copy link
@WebFreak001

WebFreak001 Nov 25, 2017

Member

This will have grammar errors after upgrading libdparse (which isn't a big problem because the unittest works, but it will eventually make the test output unreadable). Imagine the static foreach would be a static if, you can't just have function calls at a global level. Please put them in some function like void main() {} or change the body of the foreachs to declarations

Copy link
Member

left a comment

LGTM, though if anyone merges this there will be annoying error messages in the test script which could easily be considered a bug. So someone should consider updating libdparse to the latest stable version that supports static foreach first

@kotet

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2017

I confirmed that v0.7.2-alpha.3 works, but it's still an alpha version. Is that OK?

@WebFreak001

This comment has been minimized.

Copy link
Member

commented Nov 26, 2017

well if nobody else replies to this soon I will just update it to that. I don't think it really matters that much anyway as long as you use a tagged release, but I am not really active in the libdparse development so I don't really know if there are any changes in API or features someone wanted to address in this repo etc.

@wilzbach

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

Libdparse hasn't received any breaking changes. It only gets updated with the grammar being changed from time to time and sometimes to fix new compiler warnings.

@WebFreak001

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

@wilzbach is it ok if I push a libdparse update directly to master then?

@wilzbach

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

Yup. We often do this for DScanner

@WebFreak001

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

@wilzbach hm I don't have permissions to push to master, do you want to do it or should I open a PR for that?

@wilzbach

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

No one has the permission to push to master. Only passing PRs can be merged...

@kotet

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

I updated libdparse in this PR.

@kotet

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

It seems that Travis CI failed to install the compiler.

@WebFreak001

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

you didn't update the dependency in the dub.json

@kotet kotet force-pushed the kotet:issue-303 branch from 206e851 to 01a1716 Nov 28, 2017
@kotet

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

I rebased the commit.

@dlang-bot dlang-bot merged commit 4feb467 into dlang-community:master Nov 29, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kotet kotet deleted the kotet:issue-303 branch Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.