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

Don't use try-except outside MSVC #2412

Closed
wants to merge 1 commit into from

Conversation

@Crunkle
Copy link
Contributor

commented Aug 8, 2019

The try-except statements are only widely supported by MSVC. This patch will perform the memcpy without any exception handling on non-MSVC builds.

See #2407 for details.

@saghul

saghul approved these changes Aug 8, 2019

Copy link
Member

left a comment

LGTM

@bnoordhuis
Copy link
Member

left a comment

I'd prefer it if the #ifdef statements were just around the __try and __except statements, that way the memcpy logic isn't duplicated.

Also, shouldn't there be a note in the documentation somewhere?

@saghul

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

I'd prefer it if the #ifdef statements were just around the __try and __except statements, that way the memcpy logic isn't duplicated.

Ah, yeah, agreed.

Also, shouldn't there be a note in the documentation somewhere?

@Crunkle can you please add a warning note on the filemap read / write functions?

@Crunkle Crunkle force-pushed the Crunkle:v1.x branch 2 times, most recently from 0775f2f to f476db2 Aug 8, 2019

@Crunkle Crunkle force-pushed the Crunkle:v1.x branch from f476db2 to 6cc2a23 Aug 8, 2019

@Crunkle

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Sure thing, duplicates and docs done. Hopefully it is clear enough but happy to amend if not.

@saghul

saghul approved these changes Aug 8, 2019

Copy link
Member

left a comment

Perfect!

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

cjihrig added a commit that referenced this pull request Aug 9, 2019

win: remove try-except outside MSVC
Fixes: #2407
PR-URL: #2412
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Landed in 813264a. Thanks!

@cjihrig cjihrig closed this Aug 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.