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

Sample broken, Debug.Assert fails, RMW doesn't update output reference #762

Closed
tskarman opened this issue Nov 13, 2022 · 2 comments · Fixed by #759
Closed

Sample broken, Debug.Assert fails, RMW doesn't update output reference #762

tskarman opened this issue Nov 13, 2022 · 2 comments · Fixed by #759

Comments

@tskarman
Copy link

These two samples run into a failed assertion around Debug.Assert(output == value + 20);.

While this sample that is not using the ref output signature of RMW but does a separate .Read() does not fail the assertion.

From skimming the sample source it looked to me like the samples are using an implementation that is incomplete. But since the sample snippet from the docs doesn't actually use the sample source, there is prob. more going on so I'll leave the interpretation to you.

Blame links with line numbers to quickly jump to the respective samples:

Failing:

Not failing:

@badrishc
Copy link
Contributor

badrishc commented Nov 13, 2022

Good catch, the samples were assuming that the IFunctions implementation copies the new value to output, which SimpleFunctions did not. We have fixed SimpleFunctions to perform this copy to output. Note that you can always (recommended) write your own IFunctions implementation, extending from FunctionsBase or SimpleFunctions, for whatever behavior you desire. This commit fixes the issue for our in-built SimpleFunctions: 39af951 so that the samples work as they currently are.

@tskarman
Copy link
Author

thank you! :-)

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

Successfully merging a pull request may close this issue.

2 participants