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

Replace TR::Region::create() with registerDestructor() #6648

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

jdmpapin
Copy link
Contributor

@jdmpapin jdmpapin commented Aug 9, 2022

The object's type is no longer required to be copy-constructible.

The object's type is no longer required to be copy-constructible.
@0xdaryl
Copy link
Contributor

0xdaryl commented Aug 10, 2022

Jenkins build all

@0xdaryl
Copy link
Contributor

0xdaryl commented Aug 11, 2022

Seems reasonable to me. @dsouzai , do you have any concerns with this?

Copy link
Member

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

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

LGTM. In a future PR, it may be worth adding some debug code (disabled by default) that checks that the assumptions made hold (eg, obj in registerDestructor truly does belong to memory managed by the current TR::Region).

@0xdaryl 0xdaryl self-assigned this Aug 11, 2022
@0xdaryl 0xdaryl merged commit 909848f into eclipse:master Aug 11, 2022
Comment on lines +59 to +60
lastDestroyer->destroy();
lastDestroyer = lastDestroyer->prev();
Copy link
Member

Choose a reason for hiding this comment

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

Just like the old code, I think it's unwise to assume the prev link is safe to access after the object has been destroyed: I think that behavior should be restored:

   Destroyer *lastDestroyer = _lastDestroyer;
   while (lastDestroyer != NULL)
      {
      Destroyer *prev = lastDestroyer->prev();
      lastDestroyer->destroy();
      lastDestroyer = prev;
      }

Copy link
Member

Choose a reason for hiding this comment

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

lastDestroyer isn't destroyed here; it is a wrapper that contains a pointer to the object to be destroyed.

Copy link
Member

Choose a reason for hiding this comment

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

Even so, this shouldn't assume that the implementation of destroy() didn't, say, clear the prev link.

Copy link
Member

@dsouzai dsouzai Aug 15, 2022

Choose a reason for hiding this comment

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

Well, the API contract for destroy in the current impl is to only destroy the containing object, not to reset the current Destroyer's fields. The previous implementation destroyed the containing object (the Destructible object).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even so, this shouldn't assume that the implementation of destroy() didn't, say, clear the prev link.

Destroyer, TypedDestroyer, and this loop are all just part of the implementation of registerDestructor(). There is no reason for them to evolve independently, and there is therefore no significant API surface between them, and we can know (not assume) precisely what destroy() does. This objection might as well be: this shouldn't assume that the earlier part of the loop body didn't clear the prev link—to which the answer would be that it plainly doesn't.

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

Successfully merging this pull request may close these issues.

None yet

4 participants