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

Issue 14621, 14624, 14625 - Fix bugs in array operator overloading semantics #4686

Closed
wants to merge 7 commits into from

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented May 28, 2015

Issue 14621 - [REG2.066] ICE: Assertion failure: 'global.gaggedErrors || global.errors' on line 752 in file 'statement.c'
Issue 14624 - The array operator overloading fallback is not correct
Issue 14625 - opIndex() doesn't work on foreach container iteration

Shorten the length of gagged semantic path, and improve ArrayExp as the entry point of array operator overloading.

@@ -64,6 +66,8 @@ Expression *resolveAliasThis(Scope *sc, Expression *e)
e = e->semantic(sc);
}
e = resolveProperties(sc, e);

e = (gag && global.endGagging(olderrors)) ? NULL : e;
Copy link
Member

Choose a reason for hiding this comment

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

if (gag && global.endGagging(olderrors))
    e = NULL;

would be nicer IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, updated.

@9rnsr
Copy link
Contributor Author

9rnsr commented Jun 29, 2015

To @WalterBright and other committers: Please review this to fix regression Issue 14621, toward 2.068-beta2 release.

@WalterBright
Copy link
Member

I really don't like the gag technique, even though I came up with it, it's been nothing but trouble. There's got to be a better way, and there is - the TOKerror method. Also, at this point, this looks like a very large and disruptive change for this late in the beta cycle, especially since this regression was introduced well over a year ago.

@9rnsr
Copy link
Contributor Author

9rnsr commented Jul 13, 2015

Did you decide not to fix issue 14621 in 2.068 release?

…ng fallback

Points:
- Shorten the length of problematic gagged semantic path.
- Improve ArrayExp as the entry point of array operator overloading.
- Reduce stack size and the number of heap-allocated AST nodes for recursive semantic analysis.

Also fixes unlisted wrong-code issue. See test20x() in runnable/opover2.d
…bal.errors' on line 752 in file 'statement.c'
@WalterBright
Copy link
Member

Right, I think this fix is risky to insert into the 2.068 beta, not worth it because it's an old issue and hasn't caused much problems.

@9rnsr
Copy link
Contributor Author

9rnsr commented Jul 17, 2015

Ok, make sense.

@MartinNowak
Copy link
Member

If it's an old issue, please downgrade the bug from regression to something else.
We should still fix this on master.

@9rnsr
Copy link
Contributor Author

9rnsr commented Jul 21, 2015

It's not so old. The gagging issue is since 2.066, that introduced when the new operator overloading feature is added.

@9rnsr
Copy link
Contributor Author

9rnsr commented Aug 24, 2015

This PR has been moved to #4948, for the transplanting to stable branch.

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