Skip to content

Improve nvda helper by compiling with C++20 and disabling the /permissive flag#13072

Merged
feerrenrut merged 21 commits into
nvaccess:masterfrom
LeonarddeR:improveNvdaHelper
Oct 24, 2022
Merged

Improve nvda helper by compiling with C++20 and disabling the /permissive flag#13072
feerrenrut merged 21 commits into
nvaccess:masterfrom
LeonarddeR:improveNvdaHelper

Conversation

@LeonarddeR

@LeonarddeR LeonarddeR commented Nov 19, 2021 •
edited by feerrenrut
Loading

Copy link
Copy Markdown
Collaborator

Link to issue number:

None

Summary of the issue:

NVDAHelper was compiled with C++17, it could be compiled with C++20

  1. Visual Studio has support for C++20.
  2. WinRT is C++20 (Convert NVDAHelper LocalWin10 to C++/WinRT #11768).
  3. Compiling with C++20 ensures we're more conformant.
    • Passing string literals to parameters expecting wchar_t* or BSTR is no longer supported. This ensures safe code. See here for a rationale.
    • The/permissive- flag (note the minus symbol disabling permissive mode), ensuring conformance in more areas. See here

Description of how this pull request fixes the issue:

  • Change build params so nvdaHelper is built with C++20 and disable permissive mode.
  • Fix errors (wchar_t* or BSTR from string literal)

Testing strategy:

The following areas of code have been touched and need testing:

  • UIA custom automation types (comments in Excel with UIA and Windows 11)
  • Adobe reader virtual buffers
  • Gecko virtual buffers
  • MSHTML virtual buffers
  • Webkit virtual buffers

Other changes are mainly changes from char* to const char*, so unlikely to cause any harm.

I did some quick tests and didn't find anything crucial, probably best to test this in Alpha.

Known issues with pull request:

None

Change log entries:

For Developers

  • NVDAHelper is now C++20 and built with the /permissive- compiler flag, which ensures better conformance to C++ standards.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@LeonarddeR LeonarddeR changed the title Improve nvda helper by compiling with C++17 and enabling the /permissive- flag Improve nvda helper by compiling with C++20 and enabling the /permissive- flag Nov 19, 2021
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@LeonarddeR

This comment was marked as outdated.

Sign in to view
@LeonarddeR LeonarddeR marked this pull request as ready for review November 26, 2021 18:43
@LeonarddeR LeonarddeR requested a review from a team as a code owner November 26, 2021 18:43
@AppVeyorBot

This comment was marked as resolved.

Sign in to view
@feerrenrut

This comment was marked as resolved.

Sign in to view
@feerrenrut feerrenrut marked this pull request as draft February 16, 2022 10:59
@LeonarddeR LeonarddeR marked this pull request as ready for review February 18, 2022 07:17
@LeonarddeR

LeonarddeR commented Feb 18, 2022

Copy link
Copy Markdown
Collaborator Author

The build issue should have been fixed. It was due to a bad merge.

@feerrenrut

feerrenrut commented Feb 21, 2022

Copy link
Copy Markdown
Contributor

Unless there is some urgency on this that I'm not aware of, I think it would be better to merge this in the new dev cycle. Applying 2022.2 milestone.

@feerrenrut feerrenrut added this to the 2022.2 milestone Feb 21, 2022
@feerrenrut feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 18, 2022
@seanbudd

seanbudd commented May 23, 2022

Copy link
Copy Markdown
Member

We plan to merge this soon for 2022.2, and are ready to revert if there are issues

@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@LeonarddeR

LeonarddeR commented May 23, 2022

Copy link
Copy Markdown
Collaborator Author

It looks like the UIA remote library doesn't like C++20 that much. I need to dive into this some more.

@LeonarddeR LeonarddeR marked this pull request as draft May 23, 2022 08:11
@LeonarddeR LeonarddeR changed the title Improve nvda helper by compiling with C++20 and enabling the /permissive- flag Improve nvda helper by compiling with C++20 and disabling the /permissive flag May 23, 2022
LeonarddeR
LeonarddeR commented May 24, 2022
View reviewed changes
Comment thread nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp Outdated
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@seanbudd seanbudd removed this from the 2022.2 milestone Jun 16, 2022
@seanbudd seanbudd requested review from feerrenrut and removed request for michaelDCurran June 17, 2022 05:14
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@feerrenrut

This comment was marked as resolved.

Sign in to view
@feerrenrut feerrenrut added the merge-early Merge Early in a developer cycle label Jul 27, 2022
@LeonarddeR LeonarddeR marked this pull request as ready for review October 15, 2022 09:08
@LeonarddeR

LeonarddeR commented Oct 15, 2022

Copy link
Copy Markdown
Collaborator Author

This is now ready for review. Probably want to merge this early.

feerrenrut
feerrenrut reviewed Oct 21, 2022
View reviewed changes
Comment thread nvdaHelper/archBuild_sconscript
@feerrenrut feerrenrut merged commit d998806 into nvaccess:master Oct 24, 2022
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Oct 24, 2022
@LeonarddeR LeonarddeR deleted the improveNvdaHelper 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

1 more reviewer

@feerrenrut feerrenrut feerrenrut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. merge-early Merge Early in a developer cycle

Projects

None yet

Milestone

2023.1

Development

Successfully merging this pull request may close these issues.

5 participants

@LeonarddeR @AppVeyorBot @feerrenrut @seanbudd @nvaccessAuto