fix Issue 17798 - [2.076] "static foreach" not documented #1884

tgehr · 2017-09-04T13:10:12Z

Note: I was not able to build stable with make -fposix.mak. Therefore this is untested.

dlang-bot · 2017-09-04T13:10:14Z

Thanks for your pull request, @tgehr! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	17798	[2.076] "static foreach" not documented

ghost

grammar.dd needs an update too.

MetaLang · 2017-09-04T13:45:51Z

fatal: 'refs/ae-sys-d-cache/website-865504ca7123aa6b75c71a5f783b02a5e26dbcae-7bad98e9d56422f52983962526ed878a' - not a valid ref

If I recall correctly, this is a problem with Github sometimes sending the wrong commit hash. I can't remember what the workaround is... I think closing and re-opening this PR will re-run the doc tester.

tgehr · 2017-09-04T19:42:20Z

@bbasile @MetaLang Thanks!

ghost · 2017-09-04T20:57:52Z

okay thanks, looks good but i want to see the build now, it's not up to you @tgehr, you 've done the job, it's up to the build to success.

ghost · 2017-09-04T21:53:09Z

@tgehr you need to rebase and it will work. ~~Or close reopen.~~

tgehr · 2017-09-04T22:01:34Z

@bbasile There are no new commits in stable. Am I missing something?

ghost · 2017-09-04T22:05:11Z

@tgehr yes, indeed you are missing #1885 which should solve the failure.

tgehr · 2017-09-04T22:07:20Z

@bbasile: That pull request went to master, not stable. I guess this was accidental?

CyberShadow · 2017-09-04T22:08:32Z

That pull request went to master, not stable. I guess this was accidental?

Argh. Oops.

ghost · 2017-09-04T22:50:31Z

@tgehr I think that the rebase makes more sense now. Sorry for the previous failure but since i 'm the reporter on bugzilla i feel implicated in a way.

ghost · 2017-09-04T23:10:48Z

spec/version.dd

@@ -537,6 +538,127 @@ INT!(17) c;  // error, static assert trips
 	)
 	)

+$(H2 $(LNAME2 staticforeach, Static Foreach))


there's an error here i think because of the case

StaticForeach

There is also this line in the file, from which it is derived.
$(H2 $(LNAME2 staticif, Static If Condition))

I don't think whatever error there is is actually in this line.

Yeah, i may have misinterpreted the log, sorry.

WalterBright

Once it passes the autotester, of course.

PetarKirov · 2017-09-07T08:20:04Z

spec/grammar.dd

@@ -680,6 +684,7 @@ $(GNAME ForeachTypeList):
 $(GNAME ForeachType):
    $(GLINK ForeachTypeAttributes)$(OPT) $(GLINK2 declaration, BasicType) $(GLINK2 declaration, Declarator)
    $(GLINK ForeachTypeAttributes)$(OPT) $(I Identifier)
+    $(GLINK ForeachTypeAttributes)$(OPT) $(D alias) $(I Identifier)


It looks like static foreach (enum ref alias T; TypeList) is valid syntax. Is this correct?
Ditto for foreach (enum ref x; someRange).

In general, I'm wondering if we should have a separate grammar rule for [static] foreach over sequences vs the other "runtime" aggregates.

(Should ForeachTypeAttributes be allowed before alias?)

I don't think that separating grammar rules to complicate the parser makes sense. It's good for the parser to be lenient. Semantic analysis should catch it (the resulting error messages are better).

It might however be sensible to not allow ForeachTypeAttributes before alias (the reasoning was that global alias allows (some) storage classes, but it turns out that local declarations also do not allow storage classes before alias), but this pull request documents existing behaviour. You might want to open an enhancement request.

I don't think we need to change the compiler if we restructure the grammar so it is a bit harder to produce semantically invalid code.
Yes, you can (and most likely you already do) catch this during semantic analysis, however I feel it would be better for the reader of the spec if the grammar rules match more closely what is semantically valid.
Presently, the reader wouldn't have any clue that foreach (shared ref enum inout alias x; aggregate) should be rejected by the compiler - the intended semantics are not specified.

PetarKirov · 2017-09-07T08:30:03Z

spec/grammar.dd

@@ -688,20 +693,24 @@ $(GNAME ForeachTypeAttributes)
 $(GNAME ForeachTypeAttribute):
    $(D ref)
    $(GLINK2 declaration, TypeCtor)
+    $(D enum)


I wonder if enum should not be a ForeachTypeAttribute, but instead listed as a separate ForeachType entry, similar to alias.

enum is a storage class, so it would be inconsistent not to allow it.

PetarKirov · 2017-09-07T08:32:55Z

spec/grammar.dd

+
+$(GNAME StaticForeachDeclaration):
+    $(GLINK StaticForeach) $(GLINK2 attribute, DeclarationBlock)
+    $(GLINK StaticForeach) $(D :) $(GLINK2 module, DeclDefs)$(OPT)


Can you give an example for this grammar rule? Since static if doesn't support trailing colon, I'm wondering if static foreach should not support it too.

static if supports trailing colon.

Hmm, I missed that even though I searched for it, nevermind then.
(It's derived from the ConditionalDeclaration rule. For some reason I misread the spec as if it only applies for VersionCondition and DebugCondition. Sorry for the noise.)

PetarKirov · 2017-09-07T08:41:45Z

spec/version.dd

+
+	$(P The aggregate/range bounds are evaluated at compile time and
+	turned into a sequence of compile-time entities by evaluating
+	corresponding code with a $(GLINK2 statement


Missing comma at the end of the line (the link to ForeachStatement is broken).

PetarKirov · 2017-09-07T08:49:17Z

spec/version.dd

+$(GNAME StaticForeachStatement):
+    $(GLINK StaticForeach) $(GLINK2 statement, NoScopeNonEmptyStatement)
+)
+


Perhaps you should specify what happens with Ddoc when StaticForeachDeclaration declares functions preceded by a ddoc comment. Is documentation generated for each function, or is there only once instance? With static if I think there's a trick that goes along the lines of static if (condition || generatingDoc) /** This function is available iff ... */ void foo();. Another question is whether you can generate documentation and mix it in in for each declaration (e.g. declare an overload set where each function has the same documentation, save for the parameters section).

PetarKirov · 2017-09-07T08:55:00Z

spec/version.dd

+import std.conv : text;
+static foreach(i; iota(0, 3).map!text)
+{
+    mixin(`enum x`~i~`=i;`);


Currently the coding style of the spec is all over the place, but AFAIK the goal is for new code to be formatted according to DStyle (where applicable), so in this case you would need to add a space around operators (https://dlang.org/dstyle.html#phobos_whitespace).

PetarKirov · 2017-09-07T08:59:09Z

spec/version.dd

+}
+-------
+
+	$(P An explicit $(D break)/$(D continue) label can be used to avoid this limitation.


Maybe also worth making explicit is that you can't (at least you shouldn't be able to?) put a label before static foreach or any of the other conditional statement / declarations (e.g. static if).

PetarKirov · 2017-09-07T09:09:17Z

@tgehr FYI, now that CyberShadow's DAutoTest is green you can see a preview of your changes by clicking on the respective "Details" button.

MetaLang · 2017-09-12T19:58:40Z

Does this need to be squashed before merge? I remember someone saying that commits are squashed for this repo on merge but I don't know if that's actually true.

wilzbach · 2017-09-12T21:18:22Z

I remember someone saying that commits are squashed for this repo on merge but I don't know if that's actually true.

You can select the merge type once on merging (press the tiny arrow next to the merge button to switch the method) - and FWIW this is enabled for D all repos.

MetaLang · 2017-09-13T02:47:28Z

Right, but then what's the point of the auto-merge tag? Just to make sure nobody merges something broken?

wilzbach · 2017-10-05T17:38:08Z

Right, but then what's the point of the auto-merge tag? Just to make sure nobody merges something broken?

Well you can't merge anything broken (GitHub won't allow this). The point of auto-merge is that you don't need to wait until the CIs have run. There's also been auto-merge-squash, but due to "abuse" this has been temporarily removed.

Squashed it an rebased to master (that's the branch which gets auto-deployed on dlang.org - it is a constant point of confusion :S).

@ZombineDev you are still on "Requesting changes" - anything from your side that blocks this PR or can we move ahead?

PetarKirov · 2017-10-06T07:40:36Z

Let me check again

dlang-bot added the Bug Fix label Sep 4, 2017

tgehr force-pushed the fix17798 branch from da88db6 to 2a989a9 Compare September 4, 2017 13:15

tgehr changed the base branch from master to stable September 4, 2017 13:17

ghost suggested changes Sep 4, 2017

View reviewed changes

tgehr force-pushed the fix17798 branch from 50c1428 to dd31f04 Compare September 4, 2017 22:51

ghost suggested changes Sep 4, 2017

View reviewed changes

WalterBright approved these changes Sep 5, 2017

View reviewed changes

PetarKirov requested changes Sep 7, 2017

View reviewed changes

fix Issue 17798 - [2.076] "static foreach" not documented

4caec8e

wilzbach force-pushed the fix17798 branch from 5f046e9 to 4caec8e Compare October 5, 2017 17:35

dlang-bot added the Enhancement label Oct 5, 2017

wilzbach changed the base branch from stable to master October 5, 2017 17:35

PetarKirov approved these changes Oct 7, 2017

View reviewed changes

PetarKirov added the auto-merge label Oct 7, 2017

dlang-bot merged commit c190c0c into dlang:master Oct 7, 2017

fix Issue 17798 - [2.076] "static foreach" not documented #1884

fix Issue 17798 - [2.076] "static foreach" not documented #1884

Conversation

tgehr commented Sep 4, 2017 • edited Loading

dlang-bot commented Sep 4, 2017 • edited Loading

Bugzilla references

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

MetaLang commented Sep 4, 2017 • edited Loading

tgehr commented Sep 4, 2017

ghost commented Sep 4, 2017

ghost commented Sep 4, 2017 • edited by ghost Loading

tgehr commented Sep 4, 2017

ghost commented Sep 4, 2017

tgehr commented Sep 4, 2017

CyberShadow commented Sep 4, 2017

ghost commented Sep 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WalterBright left a comment

Choose a reason for hiding this comment

PetarKirov Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgehr Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

PetarKirov Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetarKirov Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetarKirov commented Sep 7, 2017 • edited Loading

MetaLang commented Sep 12, 2017

wilzbach commented Sep 12, 2017

MetaLang commented Sep 13, 2017

wilzbach commented Oct 5, 2017

PetarKirov commented Oct 6, 2017

tgehr commented Sep 4, 2017 •

edited

Loading

dlang-bot commented Sep 4, 2017 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

MetaLang commented Sep 4, 2017 •

edited

Loading

ghost commented Sep 4, 2017 •

edited by ghost

Loading

PetarKirov Sep 7, 2017 •

edited

Loading

tgehr Sep 7, 2017 •

edited

Loading

PetarKirov Sep 8, 2017 •

edited

Loading

PetarKirov Sep 7, 2017 •

edited

Loading

PetarKirov commented Sep 7, 2017 •

edited

Loading