bpo-39943: Fix MSVC warnings in sre extension #20508
Conversation
|
@petdance Your review here would be appreciated as well. |
Modules/sre_lib.h
Outdated
| @@ -453,7 +453,10 @@ do { \ | |||
| TRACE(("copy data to %p from %" PY_FORMAT_SIZE_T "d " \ | |||
| "(%" PY_FORMAT_SIZE_T "d)\n", \ | |||
| data, state->data_stack_base-size, size)); \ | |||
| memcpy(data, state->data_stack+state->data_stack_base-size, size); \ | |||
| /* We add an explicit cast to memcpy here because MSVC has a bug when | |||
There was a problem hiding this comment.
I'm not sure that it's ok to put a comment into a #define macro. Maybe convert the macro to a static inline function, or put the comment outside (before) the #define.
There was a problem hiding this comment.
Good point, this works but it might lead to weird situations like
/*
some_commented_out_code();
MACRO;
*/and then the comment suddenly gets stopped in the middle due to macro expansion.
e021e49 to
0f5ec74
Compare
0f5ec74 to
e9d75be
Compare
|
Looks fine to me. Good catch on the comments-in-a-macro. |
|
I checked Windows x64 job and I confirm that this PR fix all compiler warnings in sre files. |
|
Thanks @ammaraskar for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
|
GH-24856 is a backport of this pull request to the 3.9 branch. |
(cherry picked from commit 06e3a27) Co-authored-by: Ammar Askar <ammar@ammaraskar.com>
|
It needs to be backpoted to 3.9. |
|
See also bpo-43499. |
While the
PyMem_Delchange seems harmless enough, I'm a little bit uneasy about thememcpyone. This file is pretty old and unlikely to change in the future but it seems like it opens up the possibility of someone making a mistake likeDATA_STACK_POP(state, 5, ...)and have it not caught by the type checker because of the explicit cast.For that particular one we could guard the explicitly casted version with
#ifdef _MSC_VERor something so it's harder to make that mistake and not have it be caught by the CI.https://bugs.python.org/issue39943