Skip to content

Update to C++17, removing dependency on boost optional#9661

Merged
feerrenrut merged 9 commits intonvaccess:masterfrom
LeonarddeR:removeBoostDependency
Jan 17, 2020
Merged

Update to C++17, removing dependency on boost optional#9661
feerrenrut merged 9 commits intonvaccess:masterfrom
LeonarddeR:removeBoostDependency

Conversation

@LeonarddeR
Copy link
Copy Markdown
Collaborator

@LeonarddeR LeonarddeR commented Jun 1, 2019 •
edited
Loading

Link to issue number:

None

Summary of the issue:

Boost Optional is no longer needed, as Optional is part of the C++17standard. I started looking into how we could get rid of this dependency, even though it's not really sitting in the way. It's good to switch from something experimental to something stable, though.

Description of how this pull request fixes the issue:

We're now using optional from C++17. This required the following modifications:

  • Enable the /std:c++17 compiler flag
  • in oneCoreSpeech.cpp, rename byte to BYTE to make sure we don't conflict with std::byte
  • In COMProxyRegistration, no longer use wstring_convert<codecvt_utf8_utf16<wchar_t>>, as it is deprecated in C++17.

Testing performed:

Made sure that NVDA still runs

Known issues with pull request:

None known

Change log entry:

None needed

@LeonarddeR LeonarddeR requested a review from feerrenrut July 1, 2019 12:55
@LeonarddeR LeonarddeR force-pushed the removeBoostDependency branch from 4d30b13 to dc670ea Compare September 5, 2019 13:35
@LeonarddeR
Copy link
Copy Markdown
Collaborator Author

LeonarddeR commented Sep 6, 2019

This failed due to an artifact upload error. The tests passed just fine.

@LeonarddeR LeonarddeR requested review from michaelDCurran and removed request for feerrenrut September 6, 2019 08:06
@LeonarddeR LeonarddeR force-pushed the removeBoostDependency branch from dc670ea to 132d67f Compare October 1, 2019 09:22
@LeonarddeR LeonarddeR mentioned this pull request Jan 7, 2020
Closed
@LeonarddeR LeonarddeR changed the title Remove dependency on boost optional Update to C++17, removing dependency on boost optional Jan 7, 2020
@AppVeyorBot
Copy link
Copy Markdown

AppVeyorBot commented Jan 13, 2020

See test results for failed build of commit 1ff40c9e42

feerrenrut
feerrenrut reviewed Jan 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Please check for differences in implementation between the boost header we used and the C++17 standard. This could easily introduce subtle bugs if behavior is different between the two.

@LeonarddeR
Copy link
Copy Markdown
Collaborator Author

LeonarddeR commented Jan 14, 2020

Differences between boost optional and c++17 optional are listed in this article: https://dzone.com/articles/using-c17-stdoptional

I also got another project's pull request that did a similar replacement, see argotorg/solidity#7467. I checked for these cases and they don't apply to us. We rarely use optional actually.

@AppVeyorBot
Copy link
Copy Markdown

AppVeyorBot commented Jan 14, 2020

See test results for failed build of commit 61ccb81f8a

feerrenrut
feerrenrut reviewed Jan 15, 2020
View reviewed changes
feerrenrut
feerrenrut approved these changes Jan 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Thanks @LeonarddeR

This code was previously using the C++14 standard, the default in VS 2017.

I also checked the differences between C++14 and C++17 for any sticky issues that might affect us. I didn't notice any cause for concern. Though it's best if this change happens early in the release cycle.

@feerrenrut feerrenrut merged commit f25f33d into nvaccess:master Jan 17, 2020
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Jan 17, 2020
@codeofdusk

This comment has been minimized.

Sign in to view
@feerrenrut feerrenrut modified the milestones: 2019.3, 2020.1 Jan 20, 2020
@feerrenrut

This comment has been minimized.

Sign in to view
lukaszgo1 added a commit to lukaszgo1/nvda-misc-deps that referenced this pull request Apr 23, 2021
@lukaszgo1
Remove unused dependencies:
de68f08 
- espeakedit.exe - eSpeak NG has ability to build voice files natively
- epydoc - were using Sphinx as of nvaccess/nvda#9968
- Boost - were using optional from C++ 17 as of nvaccess/nvda#9661
@lukaszgo1 lukaszgo1 mentioned this pull request Apr 30, 2021
Merged
michaelDCurran pushed a commit to nvaccess/nvda-misc-deps that referenced this pull request Jul 27, 2021
@lukaszgo1
Remove unused dependencies (#22)
6368074 
* Remove unused dependencies:

- espeakedit.exe - eSpeak NG has ability to build voice files natively
- epydoc - were using Sphinx as of nvaccess/nvda#9968
- Boost - were using optional from C++ 17 as of nvaccess/nvda#9661

* Revert "BRLAPI for Python 3.8 (#20)"

This reverts commit 9121704.
@LeonarddeR LeonarddeR deleted the removeBoostDependency branch August 23, 2025 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran Awaiting requested review from michaelDCurran

1 more reviewer

@feerrenrut feerrenrut feerrenrut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2020.1

Development

Successfully merging this pull request may close these issues.

5 participants

@LeonarddeR @AppVeyorBot @codeofdusk @feerrenrut @nvaccessAuto