Skip to content

Add caveat for opApply and exceptions. #1224

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

Merged
merged 1 commit into from
Dec 24, 2016
Merged

Add caveat for opApply and exceptions. #1224

merged 1 commit into from
Dec 24, 2016

Conversation

Shachar
Copy link

@Shachar Shachar commented Feb 3, 2016

As discussed on the forum, add explanation about exception handling inside opApply, and their interaction with exceptions thrown from within the opApply delelgate.

@schveiguy
Copy link
Member

$(P It is important to make sure that, if $(D opApply) catches any exceptions, that those
exceptions did not origin at the delegate passed to $(I opApply). Catching an exception
that escaped from the delegate would cause a condition that the user expected would terminate
the loop to not do so.
Copy link
Member

Choose a reason for hiding this comment

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

  1. "origin at" -> originate from
  2. The last sentence is cryptic. Possibly better: "The user would expect exceptions thrown from a foreach body to both terminate the loop, and propagate outside the foreach body."

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, that appears twice: "make sure that, ..., that those"

@adamdruppe
Copy link
Contributor

@Shachar if you update the wording a bit from the comments, i'll pull this.

@AndrejMitrovic
Copy link
Contributor

Hi @Shachar, could you amend the suggested fixes? If you do not have the time we can take over. Thanks!

@andralex
Copy link
Member

I'll pull now and make the changes as a fixup.

@andralex andralex merged commit 9b1da24 into dlang:master Dec 24, 2016
andralex added a commit that referenced this pull request Dec 24, 2016
@andralex
Copy link
Member

ca83d45

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.

7 participants