Skip to content

Fix two portability defects found building with MSVC - #1408

Open
vmscvx wants to merge 1 commit into
mobile-shell:masterfrom
vmscvx:up/1-portability-fixes
Open

Fix two portability defects found building with MSVC#1408
vmscvx wants to merge 1 commit into
mobile-shell:masterfrom
vmscvx:up/1-portability-fixes

Conversation

@vmscvx

@vmscvx vmscvx commented Aug 31, 2026

Copy link
Copy Markdown

Neither is Windows-specific in itself; both are latent in the tree today.

ocb_internal.cc's ntz() passes an unsigned* where _BitScanForward takes
an unsigned long*, so the MSVC branch it lives in does not compile:

error C2664: 'unsigned char _BitScanForward(unsigned long *,unsigned
long)': cannot convert argument 1 from 'unsigned int *' to
'unsigned long *'

The file's own notes say it is tested against recent MSVC, so this looks
like drift rather than intent.

swrite.h declares a function taking a ssize_t without including anything
that defines one, and has been getting away with it because every
current includer happens to pull in <unistd.h> first. Include
<sys/types.h> so the header stands on its own.

No change to generated code on POSIX.

Neither is Windows-specific in itself; both are latent in the tree today.

ocb_internal.cc's ntz() passes an unsigned* where _BitScanForward takes
an unsigned long*, so the MSVC branch it lives in does not compile:

    error C2664: 'unsigned char _BitScanForward(unsigned long *,unsigned
    long)': cannot convert argument 1 from 'unsigned int *' to
    'unsigned long *'

The file's own notes say it is tested against recent MSVC, so this looks
like drift rather than intent.

swrite.h declares a function taking a ssize_t without including anything
that defines one, and has been getting away with it because every
current includer happens to pull in <unistd.h> first. Include
<sys/types.h> so the header stands on its own.

No change to generated code on POSIX.
@eminence

eminence commented Aug 31, 2026

Copy link
Copy Markdown
Member

What's the use-case for building with MSVC? As far as I know, this is a fully unsupported compiler for us

Edit: ah, I see from your other PR that you're working on Windows support for mosh-client. I admit that I personally have long thought that this would be possible, but I'm not sure how much Windows-specific review capability we have.

@vmscvx

vmscvx commented Sep 3, 2026

Copy link
Copy Markdown
Author

Thanks for the edit — yes, a native mosh-client on Windows is where this is going.

On this PR specifically, the two changes aren't the same kind of thing, and my title obscured that.

swrite.h has nothing to do with MSVC. The header declares a function taking a ssize_t without including anything that defines one; it builds today only because every current includer happens to pull in <unistd.h> first. Adding <sys/types.h> makes it self-contained and changes no generated code.

ntz() is in src/crypto/ocb_internal.cc, Ted Krovetz's OCB v3 reference code — the #if _MSC_VER branches came vendored with it, and its header says the code is "tested for C99 and recent versions of GCC and MSVC". So this isn't asking you to support MSVC: the MSVC branch already in the tree passes an unsigned* where _BitScanForward wants an unsigned long*, and cannot compile as written. Deleting those branches instead would be just as good an answer — say the word and I'll redo it that way, or drop that hunk and keep only the header fix.

On review capability: that seems to me the real question, and I'd rather answer it with automation than with your time. If it would help, I can add a Windows job to ci.yml that builds mosh-client with MSVC and runs the unit tests, so the build is checked by CI rather than by someone having to know Windows.

The POSIX side is meant to need no Windows knowledge to review: every change sits behind #ifdef _WIN32, the new files compile to nothing elsewhere, and make check and the clang-format 14 lint come out exactly as they do on master — I run both on Linux before pushing.

The rest of the work is split into small branches for the same reason: each is meant to be reviewable, or refusable, on its own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants