Skip to content

C#8 nullable references implementation#75

Merged
vlc-mirrorer merged 6 commits intovideolan:3.xfrom
jeremyVignelles:feature/nullable
Dec 11, 2019
Merged

C#8 nullable references implementation#75
vlc-mirrorer merged 6 commits intovideolan:3.xfrom
jeremyVignelles:feature/nullable

Conversation

@jeremyVignelles
Copy link
Copy Markdown
Contributor

Description of Change

This changes allows to use LVS with nullable annotations.

  • Added a TFM for .net standard 2.1
  • Updated LangVersion to 8.0
  • Added <Nullable>enable</Nullable> in csprojs
  • Implemented nullable references where applicable
  • Fixed possible null references exceptions
  • I also started to fix the coding style errors I was having, but once I opened a new files, a hundred more would come, so I gave up.

This is currently a Work In Progress, only LibVLCSharp and LibVLCSharp.Forms has been implemented.

Issues Resolved

API Changes

Not sure if there is any. Intent will be explained more clearly.

Platforms Affected

Not applicable

Behavioral/Visual Changes

When using LVS with nullable references enabled, Visual studio can warn you about a wrong usage of the API regarding null values.

Before/After Screenshots

Not applicable

Testing Procedure

Call every API and see if it works? 😄

PR Checklist

  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@mfkl mfkl marked this pull request as ready for review October 28, 2019 06:32
@mfkl
Copy link
Copy Markdown
Member

mfkl commented Oct 28, 2019

Whether a libvlc API can or cannot return NULL is not part of the libvlc API contract, e.g:

  • it's not documented,
  • if that changes (i.e an API that can return null becomes an API that can never return null, or the opposite), it would not be considered a breaking change by the libvlc devs and versioning. Which makes our ability to keep the binding correct very hard (not just surface API changes to track anymore).

I say we should consider all strings (and possibly any pointers) possibly null. I understand it makes the C# 8 nullable support less appealing for us, but that's just the reality of native code interop.

@mfkl
Copy link
Copy Markdown
Member

mfkl commented Oct 29, 2019

What's left to be done here?

  • Rebase
  • use nullable for all projects

@jeremyVignelles
Copy link
Copy Markdown
Contributor Author

Just

  • Rebased
  • Applied what was discussed : a pointer that comes from the native is considered nullable.

I might have missed some though...

When you say "all projects", do you mean the samples as well?

@mfkl
Copy link
Copy Markdown
Member

mfkl commented Nov 12, 2019

When you say "all projects", do you mean the samples as well?

No need to convert the sample projects, but maybe adding a small sample to libvlcsharp-samples displaying what calling a few libvlcsharp APIs with the netstandard2.1 target looks like?

@mfkl mfkl force-pushed the feature/nullable branch 2 times, most recently from 42dee63 to 97caaaa Compare December 10, 2019 06:29
@mfkl
Copy link
Copy Markdown
Member

mfkl commented Dec 10, 2019

Is this good to merge? @jeremyVignelles

@kakone
Copy link
Copy Markdown
Contributor

kakone commented Dec 10, 2019

We can now remove the #nullable enable/restore lines I added in LibVLCSharp/Shared/MediaPlayerElement files waiting this PR.

@jeremyVignelles
Copy link
Copy Markdown
Contributor Author

done. I don't think there's anything left on my side. I'm ok for merging

@vlc-mirrorer vlc-mirrorer merged commit 0a9337d into videolan:3.x Dec 11, 2019
@mfkl
Copy link
Copy Markdown
Member

mfkl commented Dec 11, 2019

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants