RFC: py/stream: Add user-streams adaptor. #2824
Conversation
For example, one can now do the following:
class S:
def write(self, n):
print('write:', repr(n))
return(len(n))
s = S()
print('abc', file=s)
|
Well, that's what I meant by "No if's and stuff" in the discussion you link to. How PinBase works, you're guaranteed to have Your patch adds requirement for (another) check. Where there were no checks at all, that adds expensive conditional jump, which may potentially stall pipelines and flush caches. To avoid that, you'd need to cache the result of But that takes extra space, and may be even not always possible (like in this patch). And the main point - |
On CPython, for a realistic cases, you'd actually need to inherit from one of |
No it doesn't. Notice that I removed a check (call to mp_get_stream_raise) so that it balances out. As I said, you need a check somewhere to see if the object has the protocol, so that check can return the adaptor if needed.
Yes, exactly, you need a check, and the check can return the protocol structure so you don't need to access it again. And it can be a user-adapted protocol if the object doesn't have a native protocol. |
|
Well, let's trace back to reconsider the facts which should be obvious, but may be they aren't. What you propose is an obvious, ground-level solution to a problem. When the problem of allowing Pins to be implemented in Python was faced, there was idea to seek for smarter solution, specifically:
This solution was achieved in the form of PinBase, which undergo review, so all facts above should have been known, and it was (and is) argued that such solution lies within Python paradigm. And duck typing isn't the only paradigm for Python, normal class inheritance plays important role too. As mentioned above, a class implementing the stream interface would realistically subclass one of io.* base classes, and there's a whole concept of ABCs. MicroPython didn't have ABCs, and PinBase is effectively how we introduce them in (unbloated uPy'esque manner). So, if you say we can do it differently - of course we can, we could have long ago. Even if "different" solution would be equivalent (and it's not), what would be the reason for change. You quoted need to poll a pin - no problem, poll is a universal operation, we can make it a part of every protocol. You quoted an (abstract) need to support multiple protocols on one object - I previously explained how that would work, there would be "GET_PROTOCOL(S)" predefined operation. If there's a need to support multiple protocol, apparently it would return not just ID, but a bitmask of supported protocols. You would say that gets complicated, and I would remind that the original goal is to have efficient C-based backbone, and then the next requirement was to try to implement Python-level interfacing with it without the need to patch this C-based infrastructure, and PinBase implements such solution, while you get back to need to patch it. |
| const mp_stream_p_t *mp_get_stream_raise(mp_obj_t self_in, int flags) { | ||
| mp_obj_type_t *type = mp_obj_get_type(self_in); | ||
| const mp_stream_p_t *stream_p = type->protocol; | ||
| if (stream_p == NULL | ||
| || ((flags & MP_STREAM_OP_READ) && stream_p->read == NULL) | ||
| || ((flags & MP_STREAM_OP_WRITE) && stream_p->write == NULL) | ||
| || ((flags & MP_STREAM_OP_IOCTL) && stream_p->ioctl == NULL)) { | ||
| // if the object is a user instance then return a protocol adaptor | ||
| if (mp_obj_is_instance_type(type)) { |
There was a problem hiding this comment.
Your patch adds requirement for (another) check. Where there were no checks at all, t
No it doesn't. Notice that I removed a check (call to mp_get_stream_raise) so that it balances out.
Here's the check that you added, bingo. Yes, you removed a check somewhere else, but counting it as a "balance" is a way of cheating ;-).
|
I think we should move on with this, as this definitely should be implemented (not necessarily enabled) to make the core "complete". So, I have the final argument for doing it as it was intended with the protocols framework, not apply opportunistic practical hacks like this patch does. So, mp_get_stream_raise() was introduced as a convenience function, it doesn't have to be used. Of course, practically, it's used, because not using it and doing adhoc checks would violate "minimal code size" requirement. BUT, one of its (always-was, always-is) legitimate uses is to pre-validate an object to have the required stream operations. Once validated, the object (and only the object) can be stored, and on the following invocations, protocol structure members can be accessed directly (that was always the idea how protocol structures should be used - they are made the way they are to minimize the runtime overhead). As this patch doesn't follow the protocol semantics and fakes the protocol structure, it doesn't work with the intended usage above. So, I'd like to extend my suggestion to redo this patch using the proper protocol inheritance, and volunteer to do such a refactor. |
|
The features in this PR were implemented in a different way using |
This allows one to write classes that act (via duck-typing) like an internal stream object. For example, one can now do the following:
The above code almost works the same on CPython, except there the argument
bufto the write method is a bytes object, rather than a bytearray.See #2747 (comment) for previous discussion leading to this.