-
Notifications
You must be signed in to change notification settings - Fork 7
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
Null annotations and in string
args
#142
Null annotations and in string
args
#142
Conversation
Azure Static Web Apps: Your stage site is ready! Visit it here: https://ambitious-hill-0f19b7c03-142.westeurope.azurestaticapps.net |
This is cool, but what do you think about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
@SupinePandora43 I like the idea about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't been active lately (in general), I'll try to fix that
{ | ||
if (!String.IsNullOrEmpty(value)) | ||
if (!String.IsNullOrEmpty(value) && Msg is not null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but C# null checker is not satisfied otherwise.
@@ -20,11 +20,21 @@ internal Lua(IntPtr ptr) | |||
|
|||
public int Top() | |||
{ | |||
if (top is null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not check it every call? Or is it eliminated by JIT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JIT will eliminate checks after few iterations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should check it. (Performance regressions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it is not optimized, here is an result with check (https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunGBLAHYYGAWQAUxAKwAeQRjQM5APgYATGABsAlBy499fAGYMx6jYtwMBAVw3a9+7p2qPXDDAAsoEAO5WYfgCiCGAwAA4YfBACYloA3A6OAL6J+qk8xADsappikvGJKdRJQA===) and without (https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHunGBLAHYYGAWQAUxAKwAeQRjQM5APgYATGABsAlBy499xAOxrNYyVoDce7gF8aNoA==). Check is just +15 CPU instructions. It is noise.
IModule current_module = (IModule)Activator.CreateInstance(t); | ||
modules.Add(current_module); | ||
gc_handles.Add(GCHandle.Alloc(current_module)); | ||
IModule? current_module = Activator.CreateInstance(t) as IModule; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it's nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateInstance has null annotation on return type: https://learn.microsoft.com/en-us/dotnet/api/system.activator.createinstance?view=net-7.0#system-activator-createinstance(system-type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add !
somewhere to make it not nullable? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why? We literally check if it is null in a next line. Let it be this way until Microsoft provide non-nulable guaranties for Activator
(so until .NET 8, maybe?).
I'll create few feature requests issues with ideas i came up with. |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://ambitious-hill-0f19b7c03-142.westeurope.azurestaticapps.net |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Removes redundant
in string
parameters from the API surface and enables nullable annotations and nullable warnings as errorsfor both GmodNET.API and core GmodNET.
Closes #141
Closes #84