Improve failing foreach reporting for struct and class aggregates

[nordlow]

Should I, similarly for enums, add tip for __traits(members)?

[ghost]

I don't think this is right. If you want to use opApply or a range .tupleof is likely not what you wanted.

[nordlow]

I don't understand what you mean with opApply or range.

What role does opApply play here and what has ranges to do with classes or structs?

Could you elaborate a bit, please?

[mihails-strasuns]

If someone has tried to do foreach over struct it is much more likely that this person expected opApply to be defined, not iterate over all fields. Thus new error message is misleading.

[nordlow]

Aha, wasn't aware of that.

What about suggesting (in error message) both my current proposal together with an alternative to add an opApply() definition?

[nordlow]

I updated the reporting to provide hints for implementing opApply aswell.

Could somebody explain why this is failing? I've not been able to run the make test on dmd because of problem mention at the top of this discussion.

[dnadlinger]

The error message should still start with, well, an actual error message. Right now, it only consists of suggested fixes. Maybe something like invalid foreach aggregate %s (define opApply member or use .tupleof to iterate over fields) would work better (disregard the actual wording, it's only a rough idea).

Still not sure about featuring tupleof so prominently, though, as the others mentioned.

[yebblies]

At a guess, it could be failing because there is a code path that returns new ErrorStatement without actually generating/printing an error.

I don't think slapping .tupleof onto an aggregate is very often the solution when you see this error.

[9rnsr]

Too verbose diagnostic message is rather harmful.

How about?

const char *s = "";
if (aggr->type && isAggregate(aggr->type))
    s = " (define opApply, range primitives,  or use .tupleof)";
error("invalid foreach aggregate %s%s", aggr->toChars(), s);

[9rnsr]

In else branch, no error is reported but this ForeachStatement::semantic will return ErrorStatement. It is the reason of Auto-tester breaking.

[nordlow]

Great! I'll look into it!

[nordlow]

I used comma as separator as this is more in line with other messages such as the one introduced in

http://dlang.org/changelog.html#warn_unused_retval

I believe this is ready for merge.

[nordlow]

This passed on all targets yesterday. Can somebody explain why it doesn't on Win64 anymore?

[yebblies]

This passed on all targets yesterday. Can somebody explain why it doesn't on Win64 anymore?

http://en.wikipedia.org/wiki/Lunar_phase

[nordlow]

:)

All is well again. BTW: Should I squash commits?

[yebblies]

Yes.

[nordlow]

Squash done.

[9rnsr]

LGTM.

[9rnsr]

Auto-merge toggled on

[nordlow]

Thanks.