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

Use references for any_cast #192

Merged
merged 1 commit into from
Sep 3, 2022
Merged

Use references for any_cast #192

merged 1 commit into from
Sep 3, 2022

Conversation

jrandolf
Copy link
Contributor

@jrandolf jrandolf commented Aug 30, 2022

This PR optimizes 'get' by returning references as opposed to values. This decreases the theoretical memory usage by 50% since values are no longer passed by value. This also let's the compiler free to optimize these calls since L-values are not returned for non-containers.

@jrandolf
Copy link
Contributor Author

@p-ranav Hey Pranav, regarding present, it currently uses std::optional, but std::optional only accepts l-values which is not great since the types returned could just be a reference. How do you feel about making it a pointer? Before you scoff too hard at it, a pointer makes sense here: It's semantically saying it could exist or be nullptr, but user should check. There is no such thing as std::optional<T&> because that would just be an std::observer_ptr<T> which itself is most likely never going to pass the committee.

@skrobinson
Copy link
Contributor

Before talking about present, I'd like to know your thinking about how get benefits by this change.

...std::optional only accepts l-values which is not great since the types returned could just be a reference.

I'm missing something here. How is a reference returned? Or is this a consequence of your proposed changes to get?

...it could exist or be nullptr, but user should check.

How is checking for nullptr better than checking for nullopt?

@jrandolf
Copy link
Contributor Author

jrandolf commented Aug 31, 2022

how get benefits by this change.

std::any carries a pointer that actually owns the element. Because it owns it, it makes more sense to return a reference and have the user coerce it into an l-value rather than doing it on behalf of the user.

How is a reference returned?

See previous answer.

Or is this a consequence of your proposed changes to get?

Yes ;)

How is checking for nullptr better than checking for nullopt?

Let's go back a bit. That question is obsolete in the larger perspective. As stated before, since we store a pointer, we should return the pointer. Forcing it into an optional duplicates memory which is doing something unnecessary on behalf of the user. So we shouldn't use std::optional. Now what can replace this? Well...std::shared_ptr or a raw pointer, but std::any owns the pointer, so all we have left is the raw pointer.

@skrobinson
Copy link
Contributor

I'll admit I don't understand all the subtleties in this PR, but the test suite passes, my argparse-using projects work with this, and in one (tiny) trial the output binary was a few bytes smaller.

I would like to see commit messages explaining why your commits are a good idea. Your reply to "...how get benefits..." would make an excellent commit message and might preclude some reviewer questions.

If you plan to add your recommended changes to present to this PR, please post those as well. If the present changes will be in a different PR, I'd be hapyy to approve this PR as it is.

@jrandolf
Copy link
Contributor Author

jrandolf commented Sep 3, 2022

The present changes will be in a different PR. That would be a breaking change.

By the way, since commit messages are generally short, I've instead opted to write the description in the PR notes.

@p-ranav p-ranav merged commit b8160a8 into p-ranav:master Sep 3, 2022
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