Repository navigation
Conversation
|
The 4th commit, I caught out of the corner of my eye as I was closing a window. 😃 |
|
The 5th commit fixes a windows build error that has probably been around for at least 2 years but wasn't noticed on CI until the other changes here. |
|
The 6th commit fixes another windows CI build error that seems unrelated to the other changes here. |
25c5618 to
f655f52
Compare
|
First commit mentions an SHA for Second commit seems to have the squashed message of other commits, but the commits themselves are still separate. Anyway maybe merge the first 2 commits since they do the same and require the same explanation, then squash introduction of STDIN_FILENO and the needed includes. |
This removes the port-specific implmentation of MP_PLAT_PRINT_STRN on the unit port. Since fee7e56, this is no longer a single function call, so we are not really optimising anything over using the default impelementation of calling mp_hal_stdout_tx_strn_cooked().
This removes the port-specific implmentation of MP_PLAT_PRINT_STRN on the windows port. This fixes releasing the GIL during the call to write() (this was missed in bc3499f). Also, mp_hal_dupterm_tx_strn() was defined but never used anywhere, so it is safe to delete it.
This replaces 0 and 1 with STDIN_FILENO and STDOUT_FILENO to make the intention of the code more clear.
This file uses STD{IN,OUT,ERR}_FILENO macros which come from this
header file.
py/stream.h uses size_t and off_t which are defined in sys/types.h
|
Thanks for the review. Having the second commit separate is intentional since the first is just a clean up and the second is fixing a mistake. And as far as I can tell, the last two commits are not directly related to the other changes but are fixing existing independent problems that were exposed as a side effect. |
Both are removing custom MP_PLAT_PRINT_STRN for essentially the same reasons I think (cleanup, not needed to have a separate definition since the mp_hal_xxx function does the same thing already)? And the windows one additionally was also wrong in the first place? I guess that could stay separate, but it's a bit confusing that the common reason isn't mentioned in both then.
Yes you're right, I missed that it was adding unistd.h for extmod, not for the ports. |
|
Thanks for this. Even though the first two commits are related I think it makes sense to have them separated as done here, eg in case they introduce regressions they are easier to track down (but I would have also been happy to merge had they been combined). Merged in b1066a9 through 2f7d2bb with some minor edits to the commit messages. |
This closes micropython#5782
3 participants
The first commit was suggested to be separated from #5723 in #5723 (comment)
The second commit is the same thing for windows.
The third commit was something I noticed while looking at the other two.