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

GetGuid noexcept Fix #3806

Merged
12 commits merged into from
Dec 3, 2019
Merged

GetGuid noexcept Fix #3806

12 commits merged into from
Dec 3, 2019

Conversation

mkitzan
Copy link
Contributor

@mkitzan mkitzan commented Dec 2, 2019

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.

References

PR Checklist

  • Closes GetGuid noexcept function throws #3763
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed updated a test group in SettingsTests
  • Requires documentation to be updated
  • 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: GetGuid noexcept function throws #3763

Detailed Description of the Pull Request / Additional comments

The noexcept specifier on GetGuid was not removed when Profile was updated 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.

Validation Steps Performed

Updated the TestHelperFunctions test group in SettingsTests and made sure the tests pass.

@mkitzan mkitzan marked this pull request as ready for review December 2, 2019 19:33
Comment on lines 85 to 88
catch (...)
{
LOG_CAUGHT_EXCEPTION();
}
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to replace this with CATCH_LOG(); I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I thought something was off with this.

@mkitzan
Copy link
Contributor Author

mkitzan commented Dec 2, 2019

Wonder what the deal is with x86 build? Stalled out at unit testing.

@DHowett-MSFT
Copy link
Contributor

/azp run

It happens sometimes 😄

@azure-pipelines
Copy link

Command 'run

It' is not supported by Azure Pipelines.



Supported commands

  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or a specific pipeline for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify a pipeline to run.
    • Example: "run" or "run pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@DHowett-MSFT
Copy link
Contributor

really?

/azp run

@mkitzan
Copy link
Contributor Author

mkitzan commented Dec 2, 2019

Thanks for the re-run 🎉

@DHowett-MSFT DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Dec 2, 2019
@ghost ghost requested review from zadjii-msft and miniksa December 2, 2019 22:45
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Yep this looks good to me!

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 3, 2019
@ghost
Copy link

ghost commented Dec 3, 2019

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit a47a53c into microsoft:master Dec 3, 2019
@mkitzan mkitzan deleted the getguid-noexcept-fix branch December 3, 2019 19:26
@ghost
Copy link

ghost commented Jan 14, 2020

🎉Windows Terminal Preview v0.8.10091.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jan 14, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetGuid noexcept function throws
4 participants