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.

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

GetGuid noexcept function throws #3763

Closed
mkitzan opened this issue Nov 28, 2019 · 5 comments · Fixed by #3806
Closed

GetGuid noexcept function throws #3763

mkitzan opened this issue Nov 28, 2019 · 5 comments · Fixed by #3806
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v0.8

Comments

@mkitzan
Copy link
Contributor

mkitzan commented Nov 28, 2019 •
edited
Loading

Environment

Windows build number: 0.0.1.0 Dev Build
Windows Terminal version (if applicable): 10.0.18363

Steps to reproduce

Invoke Profile::GetGuid on a Profile object without a GUID.

Expected behavior

GetGuid is specified as noexcept, the compiler and users expect the function to never throw.

Actual behavior

Exception thrown by a noexcept function.

New function semantics need to be decided for GetGuid and functions affected by GetGuid no longer being noexcept. Could be fixed by removing the noexcept specifier from the function and all affected functions.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 28, 2019
@zadjii-msft
Copy link
Member

You're not wrong. I bet it was never changed from noexcept back when Profiles just had a GUID, as opposed to an std::optional<GUID>

@zadjii-msft zadjii-msft added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Nov 28, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 28, 2019
@zadjii-msft zadjii-msft added this to the Terminal-1912 milestone Nov 28, 2019
@mkitzan
Copy link
Contributor Author

mkitzan commented Nov 28, 2019 •
edited
Loading

@zadjii-msft, sounds like you're going to be working in this area soon. I can make the noexcept corrections once you're finished. Until then I'll just find all the affected functions.
Edit: alternatively, GetGuid could return a std::optional<GUID> instead of throwing. However, this will require more work validating the optional and essentially deferring the throw to functions using GetGuid.

@mkitzan
Copy link
Contributor Author

mkitzan commented Nov 29, 2019

I have a branch in my fork with the changes revoking noexcept, but PR #3762 needs be merged so its code can be fixed too.

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 30, 2019
@DHowett-MSFT
Copy link
Contributor

Thanks for working on this! We'll be a bit slow on account of it's the Thanksgiving holiday here in the US, but we'll try to get through your PRs. 😄

@mkitzan mkitzan mentioned this issue Dec 2, 2019
Merged
5 tasks
@ghost ghost added the In-PR This issue has a related PR label Dec 2, 2019
@ghost ghost closed this as completed in #3806 Dec 3, 2019
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Dec 3, 2019
ghost pushed a commit that referenced this issue Dec 3, 2019
@mkitzan
GetGuid noexcept Fix (#3806)
a47a53c 
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Fixed the noexcept specifier on `GetGuid`, and corrected `FindProfile` and `FindGuid` so they don't throw. Also, adjusted `SettingsTests` to reflect these changes.

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #3763
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed updated a test group in `SettingsTests`
* [ ] Requires documentation to be updated
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #3763

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
The `noexcept` specifier on `GetGuid` was not removed when `Profile` was [updated](#3763 (comment)) to `std::optional<GUID>`. This PR fixes that and modifies two helper functions `FindProfile` and `FindGuid` in `CascadiaSettings` to work correctly if `GetGuid` does throw. 

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Updated the `TestHelperFunctions` test group in `SettingsTests` and made sure the tests pass.
@ghost
Copy link

ghost commented Jan 14, 2020

🎉This issue was addressed in #3806, which has now been successfully released as Windows Terminal Preview v0.8.10091.0.:tada:

Handy links:

donno2048 added a commit to donno2048/terminal that referenced this issue Sep 28, 2020
@donno2048
GetGuid noexcept Fix (#3806)
f378167 
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Fixed the noexcept specifier on `GetGuid`, and corrected `FindProfile` and `FindGuid` so they don't throw. Also, adjusted `SettingsTests` to reflect these changes.

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #3763
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed updated a test group in `SettingsTests`
* [ ] Requires documentation to be updated
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #3763

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
The `noexcept` specifier on `GetGuid` was not removed when `Profile` was [updated](microsoft/terminal#3763 (comment)) to `std::optional<GUID>`. This PR fixes that and modifies two helper functions `FindProfile` and `FindGuid` in `CascadiaSettings` to work correctly if `GetGuid` does throw. 

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Updated the `TestHelperFunctions` test group in `SettingsTests` and made sure the tests pass.
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v0.8
Development

Successfully merging a pull request may close this issue.

GetGuid noexcept Fix mkitzan/terminal
3 participants
@DHowett-MSFT @zadjii-msft @mkitzan