Repository navigation
Sum: fold constants without modifying shared objects, drop constants that cancel - #734
Merged
Merged
Conversation
The move-only ctor, append and prepend each repeated the assert and the in-place addition into the summand recorded by constant_summand_idx_. Route all three through a private fold_constant helper.
The move-only ctor does not clone its summands, so the constant it records may still be held by the caller, and adding later constants into it in place changed the caller's object: transform_sum_expr over a range holding Constant(1) and Constant(2) left the range's first element equal to 3. Build the folded constant as a new object and swap it in.
The move-only ctor, append and prepend omit zero summands on input but kept a constant that reached zero by folding, so Constant(1), Constant(-1) and x produced the two-term sum 0 + x although the ctor documents that zeros are omitted. Erase the recorded constant once it is zero and clear the record, so a later constant starts a new slot.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #732, stacked on its branch; retarget to master once it lands. Both fixes are pre-existing behaviors of the constant-merge path that #732 made reliably reachable.
Folding modified a constant the caller still holds. The move-only
Sumctor does not clone its summands, and later constants were added into the recorded one in place. Throughtransform_sum_expr, a range holdingConstant(1),Constant(2)andxproduced3 + xand left the range's first element equal to 3.HashingAccumulator(Wick,bernoulli.cpp) reaches the same ctor. The fold now builds a newConstantand swaps it in;append/prependwere already safe because they clone, and now share the same code.Constants that cancel were kept as a zero summand.
Constant(1),Constant(-1),xgave the two-term sum0 + x, although the ctor documents that zeros are omitted;appendandprependdid the same. The recorded constant is now erased once it is zero and the slot released, so a later constant starts a new one. A sum of nothing but cancelling constants becomes an emptySumrather thanSum{0}.Three commits: the shared
fold_constanthelper (refactor only), the non-mutating fold, and the zero drop. Each behavior commit carries a test that fails against its parent.