Eliminate data copy when encoding string with PythonEncoding (2)#941
Eliminate data copy when encoding string with PythonEncoding (2)#941BCSharp merged 5 commits intoIronLanguages:masterfrom
Conversation
slozier
left a comment
slozier
left a comment
There was a problem hiding this comment.
Thanks! Looks good to me. Testing failures are unrelated (.NET Core 2.1 testing broke on github actions updated their image and SMTP failures on Azure occur frequently -- no idea why).
| } | ||
|
|
||
| public override int GetByteCount(char[] chars, int index, int count) { | ||
| private void PrepareResidentEncoder() { |
There was a problem hiding this comment.
Might be a good candidate for [MemberNotNull(nameof(_residentEncoder))]. C# 9 isn't out yet, but I think it might work if we bump the Nullable package to 1.3.0 and maybe LangVersion to preview? Then we could get rid of all the ! below.
There was a problem hiding this comment.
It is a very nice idea. I have tried it out locally and indeed it works with Nullable at 1.3.0 and LangVersion at preview. But do we really want to set the language version to preview? As I understand it, it allows the code to use any language features from C# 9.0 without warnings, but some features from above C# 7.3 may not work on .NET Framework or .NET Core 2.x. If we accidentally use any of them, we loose compiler errors.
Anyway, there are more places that may benefit from MemberNotNullAttibute (e.g. I'm thinking of MemoryView), so I think it is better to do in a separate PR.
There was a problem hiding this comment.
On the other hand, I tried to "accidentally" use a few of those newer features and the build still fails for one reason or another. So maybe my concern is not justified.
There was a problem hiding this comment.
Yeah, I'm not too concerned about it, as you say if it's supported then it'll work, otherwise it'll fail in one way or another. We could also wait for the November release (that way we don't have to roll back preview to latest). Just don't want to forget about it.
There was a problem hiding this comment.
I'd rather wait will November. Who knows what surprises a "preview" version may have. I have set a reminder so no worry about forgetting.
It appears that .NET Core 3.1 SDK does not install any 2.x anymore. 2.2 is EOL and 2.1 is in LTS so maybe we need an extra setup action to install 2.1.22 runtime on CI? |
|
Yeah, I have a fix for the actions bit in my PR, I can merge it to master. It's the bit about SMTP that's got me stumped. |
2 participants
This is a continuation of #923, which I inadvertently closed. Commits form Aug 08 have been already reviewed by @slozier.