Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add public Charset.UTF8 and Type.UTF8Class #29968

Closed
wants to merge 1 commit into from
Closed

Conversation

luqunl
Copy link
Contributor

@luqunl luqunl commented May 29, 2018

Add Charset UTF8 in DllImport and also Type.UTF8Class

Fix https://github.com/dotnet/corefx/issues/7804

Related CoreCLR (S.P.Corelib) Change: dotnet/coreclr#18186
API approval for Charset UTF8: https://github.com/dotnet/corefx/issues/7804

@danmoseley
Copy link
Member

@luqunl since this is new public API it needs API review approval. I don't see "api-approved" on the issue https://github.com/dotnet/coreclr/issues/1012 ?

Also if we do add this, we should please have some tests in the same PR.

@luqunl
Copy link
Contributor Author

luqunl commented May 30, 2018

@danmosemsft , For API approval, please see https://github.com/dotnet/corefx/issues/7804

Also if we do add this, we should please have some tests in the same PR.

I am working on this part. currently I have to use Ildasm/ilasm to workaround roslyn support for charset UTF8.

@danmoseley
Copy link
Member

@luqunl thanks, was looking at the wrong issue.

@jkotas
Copy link
Member

jkotas commented May 30, 2018

For API approval, please see #7804

@luqunl Type.IsUTF8Class and TypeAttributes.UTF8Class are not mentioned anywhere as approved APIs in the issue #7804.

These are not just a new APIs, they are actually a ECMA format revision. It may be worth discussing whether they are really needed - what scenario they enable, etc. I am not convinced that we need them.

@karelz karelz added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 19, 2018
@karelz
Copy link
Member

karelz commented Jul 19, 2018

@luqunl what is the next step?
If there are no plans to push it forward in near future, let's close the API, until it is ready.

@karelz karelz added this to the 3.0 milestone Jul 19, 2018
@karelz
Copy link
Member

karelz commented Aug 15, 2018

Abandoned PR, closing. @luqunl feel free to reopen when you are ready to finish it off.
cc @jeffschwMSFT

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants