Skip to content

Make SimpleQuote final #2165

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

Closed
wants to merge 1 commit into from

Conversation

eltoder
Copy link
Contributor

@eltoder eltoder commented Mar 5, 2025

It's not designed for inheritance, but it is used a lot and making it final will allow compiler to devirtualize some of the calls.

It's not designed for inheritance, but it is used a lot and making it
final will allow compiler to devirtualize some of the calls.
@coveralls
Copy link

Coverage Status

coverage: 73.272%. remained the same
when pulling c587544 on eltoder:feature/simple-quote-final
into 6d6e7a7 on lballabio:master.

@lballabio
Copy link
Owner

Are you measuring any improvements? From inside a Handle<Quote>, which is the most common case and which contains a shared_ptr<Quote>, the compiler would not be able to devirtualize anyway. And when one stores a SimpleQuote, it's usually to call setValue which is already not virtual.

@eltoder
Copy link
Contributor Author

eltoder commented Mar 6, 2025

I think this will allow to devirtualize destructor calls in many cases, including when holding shared_ptr<SimpleQuote>. I'll set something up to confirm.

Copy link
Contributor

github-actions bot commented May 6, 2025

This PR was automatically marked as stale because it has been open 60 days with no activity. Remove stale label or comment, or this will be closed in two weeks.

@github-actions github-actions bot added the stale label May 6, 2025
@lballabio lballabio removed the stale label May 6, 2025
@eltoder
Copy link
Contributor Author

eltoder commented May 19, 2025

I looked into it. The improvement is limited to cases where we hold a shared_ptr<SimpleQuote> and call value() or isValid(), which is fairly rare.

The only other upside is that when getting a SimpleQuote from outside, we'll be certain that value() returns exactly the value that was passed into setValue(), but that is an even less common use case.

@eltoder eltoder closed this May 19, 2025
@lballabio
Copy link
Owner

Ok, thanks!

@eltoder eltoder deleted the feature/simple-quote-final branch May 19, 2025 18:00
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.

3 participants