Conversation
…s based on whether or not the target archetecture is the NvDA core archetecture or not, rathr than hardcoding x86 or x86_64 etc.
…ns explicit ctypes declarations for all nvdaHelperLocal functions.
… that it can be byrefed later.
…PARAM as this gives us the greatest flexibility of accepting various argument types while still ensuring the right size.
…t. The only thing that depends upon this is some old code to fetch the label of an embedded object in windows Live Mail / Outlook Express. Probbly not relevant anymore.
…t object rather than an int.
LeonarddeR
left a comment
There was a problem hiding this comment.
How broadly has the current code been tested on X86? I'm a bit worried that merging this in its current state might also have unexpected side effects to x86 builds. Wouldn't it be better to do the x64 specific work on wip-buildX64 and create x64 builds from there, then merge wip-buildX64 to master when x86 snapshots are ready to be dropped?
|
@LeonarddeR - it's not feasible to maintain a such a large diff for such a long period of time, and manage conflicts and reviews cleanly. Any instabilities in x86 machines will be resolved if possible as we transition to 64bit, but this is tested well enough to be stable enough for alphas. Merging this in ensures all future work in NVDA is 32 and 64bit compatible. Alphas should remain stable until we transition, but with alpha builds there are always risks of instability, particularly around risky changes like this. Getting this to alpha ensures our code for supporting both builds is well tested while we transition. |
|
Thanks @seanbudd, sounds reasonable to me. |
This partially reverts commit 000f182. This removes fixes to the change log for problems that weren't introduced in this PR, so they can be fixed on master.
# Conflicts: # user_docs/en/changes.md
Follow-up to #18207 Summary of the issue: Alphas are broken due to a problem with touch handler. Description of user facing changes: Alphas work again. Description of developer facing changes: None. Description of development approach: Cast `TouchHandler._wca` to `LPCWSTR` when passing to `CreateWindowExW`. This is needed because the `lpClassName` parameter of `CreateWindowExW` can accept a class atom, but ctypes doesn't know this. This wasn't previously a problem because ctypes didn't have type information for `CreateWindowExW`, so happily sent the arguments. Testing strategy: Built and self-signed NVDA. Installed and observed that NVDA didn't crash and my touchscreen worked. Known issues with pull request: None.
Related to #18207 Summary of the issue: There is unused code at `nvdaHelper/sconscript` instead we use `nvdaHelper/archBuild_sconscript` Description of user facing changes: none Description of developer facing changes: none Description of development approach: remove dead legacy code for potentially handling 64bit builds. Testing strategy: CI/CD Known issues with pull request: none
| if not isinstance(data, bytes): | ||
| data = string_at(data, size) |
There was a problem hiding this comment.
Why was this line added? Is it to prevent some errors/bugs when migrating to 64-bit?
string_at creates another copy of the data buffer stored as bytes, which is not needed, since it will only be passed to wasPlay_feed. As many built-in synths pass in c_void_p or a ctype array instead of bytes, this would create a lot of unnecessary memory copies.
data should be passed to wasPlay_feed directly if possible.
ctypes.moryview_at can be used to get a memoryview without copying the data. But it's added in Python 3.14, and wasPlay_feed doesn't need a memoryview anyway.
There was a problem hiding this comment.
Would you consider opening an issue or PR? we don't really track conversations on closed PRs very well
Link to issue number:
Summary of the issue:
NVDA needs to move to being a 64-bit application.
Description of user facing changes
None.
Description of development approach
lib.Testing strategy:
Run from source and use NVDA.
Known issues with pull request:
Various, including:
Code Review Checklist:
@coderabbitai summary