Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| /// </summary> | ||
| internal readonly struct EnergyBalanceView | ||
| { | ||
| private readonly EnergyBalanceInfoSimple balance; |
There was a problem hiding this comment.
I assume whatever tool you are using it likes to use structs a lot...
Why can't this be made simply as a IReadOnlyEnergyBalanceInfo interface? That would then be returned from the methods. That's the C# standard way to prevent modification of returned data.
There was a problem hiding this comment.
The purpose here is to avoid cast IReadOnlyEnergyBalanceInfo to EnergyBalanceInfoSimple and then execute methods like Clear() to change the readonly data. But I guess we might not need such a barrier here?
There was a problem hiding this comment.
Well obviously the data source should keep a reference to the writeable object, and only return the readonly interface to the places that are not allowed to modify it. It would be a massive hack to ever cast the read only interface back to the modifiable one, that indicates a major design problem.
There was a problem hiding this comment.
I see. So that's an over-protection here. I'll change it in the next workday.
| var compoundCreated = 0.0f; | ||
|
|
||
| foreach (var process in activeProcessList) | ||
| for (var i = 0; i < activeProcessList.Count; ++i) |
There was a problem hiding this comment.
I've used the pattern where I first assign int count = activeProcessList.Count and then in the loop use i < count. I believe this is more efficient as it reads the count property (which incurs a method call) only once and then looks at a local variable for each loop iteration. So I think this pattern could be applied in a few places that change stuff in this PR.
This branch has not been deployed
2 participants
Brief Description of What This PR Does
Prevent callers from modifying results owned by
SimulationCache. The existing public getters keep their signatures and return detached mutable copies, so changing a returned balance, process calculation or process list cannot affect subsequent cache reads.Internal consumers use readonly value views with private backing, by-value elements and struct enumerators. Cache entries remain unchanged after publication, including while an older view survives
Clear(). The calculation paths, cache keys and iteration order are preserved.The public cache getters currently expose mutable cache entries. A caller can clear a returned collection or change a result field and thereby alter later simulation reads. This change fixes that ownership leak while preserving the public mutable result types.
Note: there seems to be a hell lot of code changes, but most are just method extraction and signature changes. I think what might worth discussed about are whether there are further performance or memory improvement from the current data containers' structures.
Related Issues
No related issue.
Progress Checklist
Note: before starting this checklist the PR should be marked as non-draft.
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR). This includes gameplay testing by the PR author.
styleguide.
Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.