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

Turn Transaction into a pure interface class #13153

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ltamasi
Copy link
Contributor

@ltamasi ltamasi commented Nov 22, 2024

Summary: As groundwork for further work, the patch changes Transaction to be an abstract interface class with no data members by moving the current members log_number_, name_, txn_state_, and id_ (as well as the implementations of the methods accessing them) to TransactionBaseImpl. Note that this involves some minor API changes : SetLogNumber, GetLogNumber, GetName, SetId, and GetLastLogNumber are now pure virtual in Transaction instead of non-pure virtual, and GetState, SetState, and GetId are pure virtual instead of non-virtual. The patch also removes an unused protected Transaction constructor.

Differential Revision: D66386696

Summary: As groundwork for further work, the patch changes `Transaction` to be an abstract interface class with no data members by moving the current members `log_number_`, `name_`, `txn_state_`, and `id_` (as well as the implementations of the methods accessing them) to `TransactionBaseImpl`. Note that this involves some minor API changes : `SetLogNumber`, `GetLogNumber`, `GetName`, `SetId`, and `GetLastLogNumber` are now pure virtual in `Transaction` instead of (non-pure) virtual, and `GetState`, `SetState`, and `GetId` are pure virtual instead of non-virtual.

Differential Revision: D66386696
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66386696

Copy link
Contributor

@jowlyzhang jowlyzhang left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the change.

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.

3 participants