Remove unnecessary assignment of a value to local variables in SPC #63020
Conversation
|
Tagging subscribers to this area: @dotnet/area-meta Issue Detailsnull
|
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue Detailsnull
|
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
Is there some sort of static analysis we can enable so more of these don't get reintroduced?
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ConstructorBuilder.cs
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/Common/src/Internal/Runtime/MethodTable.cs
Outdated
Show resolved
Hide resolved
...vate.CoreLib/src/System/Threading/NativeRuntimeEventSource.PortableThreadPool.NativeSinks.cs
Outdated
Show resolved
Hide resolved
Yep, it's IDE0059 analyzer enabled but there is still more work in other libraries to enable it |
marek-safar
force-pushed
the
ide0059spc
branch
from
4a589f1 to
feb3e1a
Compare
| RuntimeTypeHandle typeOfFirstParameterIfInstanceDelegate; | ||
|
|
||
| IntPtr funcPtr = del.GetFunctionPointer(out typeOfFirstParameterIfInstanceDelegate, out bool _, out bool _); | ||
| IntPtr funcPtr = del.GetFunctionPointer(out RuntimeTypeHandle _, out bool _, out bool _); |
There was a problem hiding this comment.
Are the types necessary here (e.g. for disambiguation) or could this just have been:
IntPtr funcPtr = del.GetFunctionPointer(out _, out _, out _);?
There was a problem hiding this comment.
I believe your simpler version would work (I just kept the syntax to match)
| public static void* AllocZeroed(nuint elementCount, nuint elementSize) | ||
| { | ||
| void* result = null; | ||
| void* result; |
There was a problem hiding this comment.
Nit:
Could also be condensed to something like, e.g.
void* result = (elementCount != 0) && (elementSize != 0) ?
Interop.Sys.Calloc(elementCount, elementSize) :
Interop.Sys.Malloc(1); // the C standard does not define what happens when num == 0 or size == 0, we want an "empty" allocation
| if (_numBytes == Uninitialized) | ||
| throw NotInitialized(); | ||
|
|
||
| pointer = null; |
There was a problem hiding this comment.
This is changing behavior. If DangerousAddRef throws, previously the pointer that was passed in would have been nulled out, but now it won't. I think this should be reverted.
There was a problem hiding this comment.
(I opened an issue on Roslyn for this exact case almost two years ago: dotnet/roslyn#42761)
| } | ||
|
|
||
| bool retVal = true; | ||
| bool retVal; |
There was a problem hiding this comment.
Rather than defining this all the way up here, each of the two call sites later on that use this could be changed, e.g. from
retVal = WaitOnEvent(_waitUpgradeEvent, ref _numWriteUpgradeWaiters, timeout, EnterLockType.UpgradeToWrite);
// The lock is not held in case of failure.
if (!retVal)
return false;to
bool retVal = WaitOnEvent(_waitUpgradeEvent, ref _numWriteUpgradeWaiters, timeout, EnterLockType.UpgradeToWrite);
// The lock is not held in case of failure.
if (!retVal)
return false;and then the local could be further removed entirely:
// The lock is not held in case of failure.
if (!WaitOnEvent(_waitUpgradeEvent, ref _numWriteUpgradeWaiters, timeout, EnterLockType.UpgradeToWrite))
return false;
ghost
locked as resolved and limited conversation to collaborators
4 participants
No description provided.