Run ctypes tests on macOS #1913
Conversation
| case SimpleTypeKind.Boolean: owner.WriteByte(offset, ModuleOps.GetBoolean(value, this)); break; | ||
| case SimpleTypeKind.Char: owner.WriteByte(offset, ModuleOps.GetChar(value, this)); break; | ||
| case SimpleTypeKind.SignedByte: owner.WriteByte(offset, ModuleOps.GetSignedByte(value, this)); break; | ||
| case SimpleTypeKind.SignedByte: owner.WriteByte(offset, (byte)ModuleOps.GetSignedByte(value, this)); break; |
There was a problem hiding this comment.
Maybe add an unchecked. I know it's unchecked by default but I like to be explicit about it...
There was a problem hiding this comment.
Ha!, I thought you would comment on my converting IntPtr to void*. 😃
About narrowing casts I think we are on the same page — I, too, prefer to be explicit about the intentions. As a rule, I always put a narrowing cast in a checked or unchecked guard, regardless how the project is being compiled by default — except if from the nature of the data or the program logic it is clear that no overflow is possible (an int containing a char, for example).
Here I have those casts guarded as unchecked already in the follow up commits in #1912; If you don't mind, I prefer to keep them there: If I did it here, the temptation would be to add unchecked to other preexisting casts in this switch statement (or farther), and not doing it would raise more questions ("why unchecked here but not there?"). In #1912 more unchecked casts are coming, so let's do them together. Also, here I just wanted to limit any changes to specifically what was required to fix marshalling bugs on ARM, so the less changes the better for my future self or whoever looking back trying to understand the whats and whys.
There was a problem hiding this comment.
As a rule, I always put a narrowing cast in a
checkedoruncheckedguard,
I see that I was not following this rule in #1441. OK, let's call it a guideline… 😃
There was a problem hiding this comment.
Here I have those casts guarded as unchecked already in the follow up commits in #1912;
Fine by me. I guess the other PR will keep the return type of GetUnsignedShort as a short so there's no casting issue at this call site?
I've been meaning to try an turn on CheckForOverflowUnderflow at some point to see if it would find any errors. Unfortunately I can never seem to find the time to actually try it...
There was a problem hiding this comment.
I guess the other PR will keep the return type of
GetUnsignedShortas ashortso there's no casting issue at this call site?
I don't think I understand what you mean. Did you mean :
the other PR will keep the return type of
GetUnsignedShortas aushort?
I changed the return type exactly because casting from a short returned from GetUnsignedShort call in the runtime-gerenated marshalling code was causing the sign bit extended incorrectly. I guess this may be one of those Arm/Intel differences. In #1912 I intended to change more return signatures and then add unchecked() guards where appropriate. Anyway, we can continue this discussion in #1912, when I resubmit it.
| using Mono.Unix.Native; | ||
|
|
||
| using IronPython.Runtime; | ||
| using System.Runtime.Versioning; |
There was a problem hiding this comment.
Maybe sort the usings if you feel like it. 😄
| Ignore=true | ||
|
|
||
| [CPython.ctypes.test_frombuffer] | ||
| RunCondition=NOT $(IS_MONO) |
There was a problem hiding this comment.
I'm fine disabling on Mono, but I guess it was running before on Linux?
There was a problem hiding this comment.
Well, sort of, not really… It depends on when the GC runs the finalizers and on Mono it is unpredictable. So the test sometimes works and sometimes fails. This happens more likely on my ARM64 than CI, I've noticed, not sure why. Perhaps CI, as more memory-constrained, prompts Mono to run the GC more aggressively, so it just happens to work.
Since we have already way to many or too frequently false positives to the point of it being frustrating, whenever I see this pattern that Mono is not running finalizers on GC.Collect() I disable the test on Mono, with the appropriate comment. Once Mono releases a version with a fixed implementation of GC.Collect() (if it ever releases any new version), we can re-enable those tests.
In this particular example it was possible to create test_frombuffer_stdlib and, just for Mono, run safe tests selectively, but, frankly, I have very little patience for Mono. The disabled test should not be framework-specific so .NET tests should do decent work anyway.
Indeed. Here I am, happily hacking away thinking I have the safety net of the tests, and, to my surprise, discovering that none of the |
2 participants
It also adds a
.pydtest module for architecture aarch64 on Linux.