Skip to content

Fixup on «mg: fix deferred element ownership» - #25039

Open
Leont wants to merge 1 commit into
bleadfrom
defelem
Open

Leont wants to merge 1 commit into
bleadfrom
defelem

Conversation

@Leont

@Leont Leont commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

29974cc was accidentally pushed to blead without review. It has several issues that should have been fixed before being merged.

  1. Saving old_target makes sense, saving old_key does not; its decrement is already after the targs increment.
  2. For readability its desirable if the decrement of old_target happens directly after the increment of the new target, because that way all the target handling code is together.
  3. In Perl_vivify_defelem there's a repeat of this dance that doesn't seem to serve any purpose (the increment is already before the decrement), it appears to be just a hallucination.
  4. Setting value = NULL is unnecessary because value is already initialized to NULL, so that whole branch could be simplified.
  5. The tests for this do not belong in t/op/for.t, I moved them to t/op/avhv.t instead.
  6. Most of the comments don't make sense because they appear to be about the change and not about the code.

29974cc was accidentally pushed to
blead without review. It has several issues that should have been fixed
before being merged.

* Saving old_target makes sense, saving old_key does not; its decrement
  is already after the targs increment.
* For readability its desirable if the decrement of old_target happens
  directly after the increment of the new target, because that way all
  the target handling code is together.
* In Perl_vivify_defelem there's a repeat of this dance that doesn't
  seem to serve any purpose (the increment is already before the
  decrement), it appears to be just a hallucination.
* Setting value = NULL is unnecessary because value is already
  initialized to NULL, so that whole branch could be simplified.
* The tests for this do not belong in t/op/for.t, I moved them to
  t/op/avhv.t instead.
* Most of the comments don't make sense because they appear to be about
  the change and not about the code.
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.

1 participant