Closed martindevans closed 4 months ago
Looks like you have some build issues to fix before this can be merged. No concerns with the changes themselves.
Oops sorry about that, I was only testing under net8.0
, not netstandard2.0
. I've reverted that change.
Regarding the nullability issues, how do you want me to approach those? I know some people prefer to keep the nullability checks, even when they're not strictly necessary, because in theory someone could use the library from a project with nullability checks disabled. My personal opinion is to go "all in" on nullability checks, and assume callers will be using them properly.
All-in on nullability is my preference, too. Annotations are not binary-breaking changes, so anyone who has a transient dependency to this project will be unaffected. It's just a minor inconvenience for developers during their builds.
Fixed various minor code quality issues, as suggested by ReSharper. Hopefully all of these are fairly "non controversial" changes:
x.First(y)
instead ofx.Where(y).First()
in FunctionTable.cs, UnmanagedMemory.cs[]
instead ofArray.Empty<T>
in BlockStack.cs, Signature.cs, RuntimeImport.csSection
cannot be <Section.None
)using
in Branch.cs, BranchIf.cschecked
context in Compile.cssealed
keywords (on methods, where the whole class is sealed) in CompilationContext.cs, GlobalInfo.csThere are a large number of nullability issues of the type
Expression is always false according to nullable reference types' annotations
orConditional access qualifier expression is never null according to nullable reference types' annotations
. Are you happy for me to fix all of these in a followup PR? I'll inspect each case and either remove the redundant null checks, or add a nullability annotation where appropriate.