Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "Pad*" signatures with more honest nullable annotations #804

Merged
merged 5 commits into from
Mar 22, 2021

Conversation

atifaziz
Copy link
Member

This PR adds to #803. Where a TSource is defaulted for padding, the signature now clearly says TSource?.

Copy link
Contributor

@viceroypenguin viceroypenguin left a comment

Choose a reason for hiding this comment

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

Idle curiosity on NRT handling: I wonder if passing a Func<int, T> will return an IEnumerable<T> or IEnumerable<T?>. Looks good regardless

@atifaziz
Copy link
Member Author

atifaziz commented Mar 19, 2021

Idle curiosity on NRT handling: I wonder if passing a Func<int, T> will return an IEnumerable<T> or IEnumerable<T?>.

Will return based on whatever T is for the instantiation.

Looks good regardless

Almost. Had one of the overloads wrong but hopefully fixed with 58ca7f9.

@viceroypenguin
Copy link
Contributor

Without the Defaultable<T>, it complains? Is it better to add the Defaultable<T> or to just ! it away? Seems like a lot of work to avoid a single character.

@atifaziz
Copy link
Member Author

Without the Defaultable<T>, it complains?

Yes.

Is it better to add the Defaultable<T> or to just ! it away?

I'll admit I'm being sneaky with the compiler, but I am toying with the idea and wanted to throw it here to get reactions from reviewers so thank you for yours!

Seems like a lot of work to avoid a single character.

JIT should optimise Defaultable<T> away 100% so it should have zero runtime impact. Putting that technical detail aside, my goal here was not to save on the single character, but:

  1. not having to default! at all every call site (usually less important).
  2. making it obvious in the (always private) method signature that the argument may be defaulted due to mutual exclusion with some other argument.
  3. quickly find all references of Defaultable<T> at some point in the future for code analysis.

While it might seem extreme for Pad, I am hoping it will help in other cases like FallbackIfEmpty. If it ends up being a bad or an over-the-top idea then one can find all references (3) and revert.

@atifaziz atifaziz changed the title Fix "Pad" signatures with more honest nullable annotations Fix "Pad*" signatures with more honest nullable annotations Mar 19, 2021
Copy link
Contributor

@viceroypenguin viceroypenguin left a comment

Choose a reason for hiding this comment

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

I'm good w/ the explicit Defaultable<T>. Definitely agree that it will JIT away, was more on the "why create it" aspect. If it works, then 🤷‍♂️

@atifaziz atifaziz merged commit ed0cf71 into morelinq:master Mar 22, 2021
@atifaziz atifaziz deleted the fix/nrt/Pad branch March 22, 2021 21:47
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