Skip to content

Replace custom optional with std::optional #9068

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 11, 2025
Merged

Replace custom optional with std::optional #9068

merged 6 commits into from
Mar 11, 2025

Conversation

swolchok
Copy link
Contributor

@swolchok swolchok commented Mar 8, 2025

We have C++17, no reason to keep this old thing around. Had to resolve some ambiguous overloads.

@swolchok
Update
a54e050 
[ghstack-poisoned]
@swolchok
Copy link
Contributor Author

swolchok commented Mar 8, 2025 •
edited
Loading

Stack from ghstack (oldest at bottom):

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Mar 8, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9068

Note: Links to docs will display an error until the docs builds have been completed.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 8, 2025
swolchok added a commit that referenced this pull request Mar 8, 2025
@swolchok
Replace custom optional with std::optional
1c1d104 
We have C++17, no reason to keep this old thing around. Had to resolve some ambiguous overloads.

ghstack-source-id: 070a4ed
ghstack-comment-id: 2707860333
Pull Request resolved: #9068
@facebook-github-bot
Copy link
Contributor

@swolchok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@swolchok
Update
7364de3 
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Mar 10, 2025
@swolchok
Replace custom optional with std::optional
5d7c33b 
We have C++17, no reason to keep this old thing around. Had to resolve some ambiguous overloads.

ghstack-source-id: bb307e6
ghstack-comment-id: 2707860333
Pull Request resolved: #9068
@facebook-github-bot
Copy link
Contributor

@swolchok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@swolchok
Update
3e3c632 
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Mar 10, 2025
@swolchok
Replace custom optional with std::optional
ddf914f 
We have C++17, no reason to keep this old thing around. Had to resolve some ambiguous overloads.

ghstack-source-id: 20257c2
ghstack-comment-id: 2707860333
Pull Request resolved: #9068
@swolchok
Copy link
Contributor Author

ping reviewer @JacobSzwejbka

JacobSzwejbka
JacobSzwejbka approved these changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@JacobSzwejbka JacobSzwejbka left a comment

Choose a reason for hiding this comment

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

Hmm I feel like I've been asked by like 4 different people about removing this, I guess none of them ever put up a PR. Thanks for doing it

@swolchok
Update
7787aa7 
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Mar 11, 2025
@swolchok
Replace custom optional with std::optional
8bf56a0 
We have C++17, no reason to keep this old thing around. Had to resolve some ambiguous overloads.

ghstack-source-id: b5454e6
ghstack-comment-id: 2707860333
Pull Request resolved: #9068
@facebook-github-bot
Copy link
Contributor

@swolchok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@swolchok
Update
02b1669 
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Mar 11, 2025
@swolchok
Replace custom optional with std::optional
58c8891 
We have C++17, no reason to keep this old thing around. Had to resolve some ambiguous overloads.

ghstack-source-id: def5132
ghstack-comment-id: 2707860333
Pull Request resolved: #9068
@facebook-github-bot
Copy link
Contributor

@swolchok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@swolchok
Update
dab0a36 
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Mar 11, 2025
@swolchok
Replace custom optional with std::optional
ff2b544 
We have C++17, no reason to keep this old thing around. Had to resolve some ambiguous overloads.

ghstack-source-id: fa6120c
ghstack-comment-id: 2707860333
Pull Request resolved: #9068
@swolchok swolchok merged commit b9f10d4 into main Mar 11, 2025
4 of 5 checks passed
@swolchok swolchok deleted the gh/swolchok/330/head branch March 11, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JacobSzwejbka JacobSzwejbka JacobSzwejbka approved these changes

@lucylq lucylq Awaiting requested review from lucylq lucylq is a code owner

@manuelcandales manuelcandales Awaiting requested review from manuelcandales manuelcandales is a code owner

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: not user facing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@swolchok @facebook-github-bot @JacobSzwejbka