Skip to content

Implement Bytes.__new__#943

Merged
BCSharp merged 4 commits intoIronLanguages:masterfrom
BCSharp:bytes_ctor
Sep 24, 2020
Merged

Implement Bytes.__new__#943
BCSharp merged 4 commits intoIronLanguages:masterfrom
BCSharp:bytes_ctor

Conversation

@BCSharp
Copy link
Copy Markdown
Member

@BCSharp BCSharp commented Sep 22, 2020

This is primarily to support the following case (from stdlib tests):

class BytesSubclass(bytes):
    pass

class SomeClass:
    def __bytes__(self):
        return BytesSubclass(b'abc')

assert type(bytes(SomeClas())) is BytesSubclass

Also an opportunity to clean up constructor overloads in Bytes.

@BCSharp BCSharp requested a review from slozier September 22, 2020 04:38
Copy link
Copy Markdown
Contributor

@slozier slozier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems generally good.

Not that I necessarily think this PR needs to be amended for these cases (since we weren't supporting them before), but we're missing the "always call __bytes__" whenever we hit specialized __new__ overloads:

class BytesSubclass(bytes):
  def __bytes__(self):
    return BytesSubclass(b"x")

x = bytes(BytesSubclass(b"y"))
assert type(x) is BytesSubclass and x == b"x"

class Test(str):
  def __bytes__(self):
    return b"bytes"

assert Test("str") == b"bytes"

if (!isLittle && byteorder != "big") throw PythonOps.ValueError("byteorder must be either 'little' or 'big'");

return FromBytes(new Bytes(context, bytes), isLittle, signed);
return FromBytes((Bytes)Bytes.__new__(context, TypeCache.Bytes, bytes), isLittle, signed);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling __new__ from C# seems a bit icky. I wonder if we should have a Bytes Bytes.FromObject(object) helper instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't fond of it either. Will try to factor it out.

@BCSharp
Copy link
Copy Markdown
Member Author

BCSharp commented Sep 23, 2020

Oh, I forgot about __bytes__ from bytes... I tested it with other types though (list, str). I intended this PR to get the bytes construction right so I will amend it.

assert Test("str") == b"bytes"

I assume you meant

assert bytes(Test("str")) == b"bytes"

This one throws on CPython 3.4 (which is the one I use for testing for IronPython 3) but now I see it works under CPython 3.5. It looks to me like an ommision in CPython 3.4 that got rectified in the next release so I am going to implement the later behaviour.

Copy link
Copy Markdown
Contributor

@slozier slozier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me assuming the tests pass. Just a minor suggestion.

Comment thread Src/IronPython/Runtime/Bytes.cs Outdated
public static object __new__(CodeContext context, [NotNull] PythonType cls, [NotNull] IBufferProtocol source) {
if (cls == TypeCache.Bytes) {
return new Bytes(source);
if (TryInvokeBytesOperator(context, source, out Bytes? res)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an if (source.GetType() == typeof(Bytes)) return source;? Could use the same optimization in FromObject.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a separate overload?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What sort of overload?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 05ceb50 I added an overload for the C# constructor with a Bytes parameter as a fast track. I was also thinking about an overload for the Python __new__ constructor, but this would not work: it would catch also the subclasses and those have to be tried with __bytes__.

@BCSharp BCSharp merged commit f6bffda into IronLanguages:master Sep 24, 2020
@BCSharp BCSharp deleted the bytes_ctor branch September 24, 2020 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants