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

Realias DiffResults whenever possible #261

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gdalle
Copy link
Member

@gdalle gdalle commented Jul 28, 2024

Fix #251 partially.

  • Do result = operator!(result, f, x) instead of operator!(result, f, x) whenever possible (see Dependents who forget to re-alias results DiffResults.jl#27)
  • There are still some parts involving closures which I don't know how to handle.
  • Adjust the tests accordingly
  • Add new tests that would fail on the current release.

@devmotion do you know the package well enough to help me figure out what I missed?

@devmotion
Copy link
Member

I'm off until next week and probably won't be able to add a meaningful comment until then.

@gdalle
Copy link
Member Author

gdalle commented Jul 31, 2024

No worries, I'll wait for your opinion!

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

At first glance, my impression is that there's more work to be done for full StaticArrays support than just fixing the incorrect use of DiffResults (see e.g. #154).

I'm not a fan of major changes to tests in PRs with such significant changes either, so I suggest splitting the PR into multiple smaller PRs, e.g., one that adds and/or modifies tests (without changing the package) and one that fixes the DiffResults API usage.

Given that static arrays have additional problems, I would also suggest just testing ImmutableDiffResult and MutableDiffResult with Arrays (and not SArrays or MArrays) initially (I assume that this will also cause problems currently if the DiffResults API is used incorrectly).

To increase readability, I think we might also want to use the bangbang notation (!!) for internal functions that are changed to be only sometimes mutable (I'd refrain from changing the user-facing API right now).

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 this pull request may close these issues.

DiffResults objects are not re-aliased properly
2 participants