Skip to content

Multiple improvements by CodeRush Static Analysis #5132

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.

Sign up for GitHub

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

Merged
TravisEz13 merged 2 commits into from
Nov 10, 2017
Merged

Multiple improvements by CodeRush Static Analysis #5132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b829325
Multiple improvements by CodeRush Static Analysis
Himura2la Oct 15, 2017
4effc48
Changes to PR from the Team
Himura2la Oct 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/Microsoft.PowerShell.Commands.Management/commands/management/GetChildrenCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,8 +288,7 @@ protected override void ProcessRecord()
{
CmdletProviderContext currentContext = CmdletProviderContext;

if (_paths == null ||
(_paths != null && _paths.Length == 0))
if (_paths == null || _paths.Length == 0)
{
_paths = new string[] { String.Empty };
}
Expand Down
7 changes: 2 additions & 5 deletions src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2362,8 +2362,7 @@ protected override bool ProviderSupportsShouldProcess
/// </summary>
protected override void ProcessRecord()
{
if (Path == null ||
(Path != null && Path.Length == 0))
if (Path == null || Path.Length == 0)
{
Path = new string[] { String.Empty };
}
Expand Down Expand Up @@ -4533,9 +4532,7 @@ public string[] PSProvider
/// </summary>
protected override void ProcessRecord()
{
if (PSProvider == null ||
(PSProvider != null &&
PSProvider.Length == 0))
if (PSProvider == null || PSProvider.Length == 0)
{
// Get all the providers

Expand Down
3 changes: 1 addition & 2 deletions src/Microsoft.PowerShell.Commands.Management/commands/management/SetContentCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ public class SetContentCommand : WriteContentCommandBase
///
internal override void BeforeOpenStreams(string[] paths)
{
if (paths == null ||
(paths != null && paths.Length == 0))
if (paths == null || paths.Length == 0)
{
throw PSTraceSource.NewArgumentNullException("paths");
}
Expand Down
6 changes: 2 additions & 4 deletions src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostRawUserInterface.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,8 +741,7 @@ public override
if (((ConsoleControl.InputRecordEventTypes)inputRecords[0].EventType) ==
ConsoleControl.InputRecordEventTypes.KEY_EVENT)
{
Dbg.Assert((inputRecords[0].KeyEvent.KeyDown && inputRecords[0].KeyEvent.RepeatCount != 0) ||
!inputRecords[0].KeyEvent.KeyDown,
Dbg.Assert(!inputRecords[0].KeyEvent.KeyDown || inputRecords[0].KeyEvent.RepeatCount != 0,
string.Format(CultureInfo.InvariantCulture, "ReadConsoleInput returns a KeyEvent that is KeyDown and RepeatCount 0"));
if (inputRecords[0].KeyEvent.RepeatCount == 0)
{
Expand Down Expand Up @@ -867,8 +866,7 @@ public override
if (((ConsoleControl.InputRecordEventTypes)inputRecords[i].EventType) ==
ConsoleControl.InputRecordEventTypes.KEY_EVENT)
{
Dbg.Assert((inputRecords[i].KeyEvent.KeyDown && inputRecords[i].KeyEvent.RepeatCount != 0) ||
!inputRecords[i].KeyEvent.KeyDown,
Dbg.Assert(!inputRecords[i].KeyEvent.KeyDown || inputRecords[i].KeyEvent.RepeatCount != 0,
string.Format(CultureInfo.InvariantCulture, "PeekConsoleInput returns a KeyEvent that is KeyDown and RepeatCount 0"));

if (inputRecords[i].KeyEvent.KeyDown && inputRecords[i].KeyEvent.RepeatCount == 0)
Expand Down
11 changes: 1 addition & 10 deletions src/Microsoft.PowerShell.CoreCLR.Eventing/DotNetCode/Eventing/Reader/EventLogReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,16 +305,7 @@ public void Seek(SeekOrigin origin, long offset)
}
else
{
//if inside the current buffer, we still cannot read the events, as the handles.
//may have already been closed.
if (_currentIndex + offset >= 0)
{
SeekCommon(offset);
}
else //outside the current buffer
{
SeekCommon(offset);
}
SeekCommon(offset);
}
return;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.WSMan.Management/ConfigProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2581,7 +2581,7 @@ private void PutResourceValue(object sessionobj, string ResourceURI, Hashtable v
nsmgr.AddNamespace("cfg", uri_schema);
string xpath = SetXPathString(ResourceURI);
XmlNodeList nodelist = inputxml.SelectNodes(xpath, nsmgr);
if (nodelist.Count == 1 && nodelist != null)
if (nodelist != null && nodelist.Count == 1)
{
XmlNode node = (XmlNode)nodelist.Item(0);
if (node.HasChildNodes)
Expand Down
15 changes: 1 addition & 14 deletions src/Microsoft.WSMan.Management/WsManHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,20 +495,7 @@ internal string ProcessInput(IWSManEx wsman, string filepath, string operation,

internal string GetXmlNs(string resUri)
{

string tmpNs = null;

if (resUri.ToLowerInvariant().Contains(URI_IPMI) || (resUri.ToLowerInvariant().Contains(URI_WMI)))
tmpNs = StripParams(resUri);
else
{
//tmpNs = StripParams(resUri) + ".xsd";
//This was reported by Intel as an interop issue. So now we are not appending a .xsd in the end.
tmpNs = StripParams(resUri);
}

return (@"xmlns:p=""" + tmpNs + @"""");

return (@"xmlns:p=""" + StripParams(resUri) + @"""");
}

internal XmlNode GetXmlNode(string xmlString, string xpathpattern, string xmlnamespace)
Expand Down
5 changes: 1 addition & 4 deletions src/System.Management.Automation/engine/CommandCompletion/CommandCompletion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1182,10 +1182,7 @@ private string FindLastWord(out int replacementIndexOut, out char closingQuote)

for (_sentenceIndex = 0; _sentenceIndex < _sentence.Length; ++_sentenceIndex)
{
Diagnostics.Assert(
(inSingleQuote && !inDoubleQuote)
|| (inDoubleQuote && !inSingleQuote)
|| (!inSingleQuote && !inDoubleQuote),
Diagnostics.Assert(!(inSingleQuote && inDoubleQuote),
"Can't be in both single and double quotes");

char c = _sentence[_sentenceIndex];
Expand Down
7 changes: 1 addition & 6 deletions src/System.Management.Automation/engine/CoreAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -862,10 +862,7 @@ private static int CompareOverloadCandidates(OverloadCandidate candidate1, Overl
return 0;
}
}
}

if (betterCount == 0)
{
// Apply tie breaking rules, related to expanded parameters
if (candidate1.expandedParameters != null && candidate2.expandedParameters != null)
{
Expand All @@ -880,10 +877,8 @@ private static int CompareOverloadCandidates(OverloadCandidate candidate1, Overl
{
return 1;
}
}

if (betterCount == 0)
{

// Apply tie breaking rules, related to specificity of parameters
betterCount = CompareTypeSpecificity(candidate1, candidate2);
}
Expand Down
26 changes: 5 additions & 21 deletions src/System.Management.Automation/engine/DataStoreAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,18 +804,9 @@ public bool Equals(PSDriveInfo drive)
Object drive1Object = drive1;
Object drive2Object = drive2;

if ((drive1Object == null))
if (drive1Object == null)
{
if (drive2Object == null)
{
// Since both drives are null, they are equal
return false;
}
else
{
// Since drive1 is null it is less than drive2 which is not null
return true;
}
return (drive2Object != null);
}
else
{
Expand Down Expand Up @@ -856,16 +847,9 @@ public bool Equals(PSDriveInfo drive)

if ((drive1Object == null))
{
if (drive2Object == null)
{
// Since both drives are null, they are equal
return false;
}
else
{
// Since drive1 is null it is less than drive2 which is not null
return false;
}
// Since both drives are null, they are equal
Copy link
Contributor

@PaulHigin PaulHigin Oct 16, 2017

Choose a reason for hiding this comment

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

This doesn't look right. I believe it should be.
return (drive2Object != null);

Copy link
Contributor Author

@Himura2la Himura2la Oct 17, 2017

Choose a reason for hiding this comment

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

Oh, thanks, I didn't notice that the refactoring can go even further

// Since drive1 is null it is less than drive2 which is not null
return false;
}
else
{
Expand Down
6 changes: 0 additions & 6 deletions src/System.Management.Automation/engine/DataStoreAdapterProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,12 +400,6 @@ internal ProviderInfo(
throw PSTraceSource.NewArgumentNullException("implementingType");
}

if (String.IsNullOrEmpty(name))
{
throw PSTraceSource.NewArgumentException("name");
}


if (String.IsNullOrEmpty(name))
{
throw PSTraceSource.NewArgumentException("name");
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/LanguagePrimitives.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3691,7 +3691,7 @@ internal object Convert(object valueToConvert,
ExecutionContext ecFromTLS = LocalPipeline.GetExecutionContextFromTLS();
object result = null;

if ((ecFromTLS == null) || (ecFromTLS != null && ecFromTLS.LanguageMode == PSLanguageMode.FullLanguage))
if (ecFromTLS == null || ecFromTLS.LanguageMode == PSLanguageMode.FullLanguage)
{
result = _constructor();
var psobject = valueToConvert as PSObject;
Expand Down
4 changes: 2 additions & 2 deletions src/System.Management.Automation/engine/Modules/AnalysisCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,14 +634,14 @@ public void QueueSerialization()
// Wait a while before assuming we've finished the updates,
// writing the cache out in a timely matter isn't too important
// now anyway.
await Task.Delay(10000);
await Task.Delay(10000).ConfigureAwait(false);
Copy link
Contributor

@PaulHigin PaulHigin Oct 16, 2017

Choose a reason for hiding this comment

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

It is not clear to me that this is needed. Isn't the Task always configured as "false" be default?

Copy link
Contributor

@lzybkr lzybkr Oct 16, 2017

Choose a reason for hiding this comment

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

It seems right though, so I think it's good to keep it.

Copy link
Contributor Author

@Himura2la Himura2la Oct 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@PaulHigin As far as I know, the default option is 'true'. And that's the reason for adding an explicit ConfigureAwait() everywhere.

int counter1, counter2;
do
{
// Check the counter a couple times with a delay,
// if it's stable, then proceed with writing.
counter1 = _saveCacheToDiskQueued;
await Task.Delay(3000);
await Task.Delay(3000).ConfigureAwait(false);
counter2 = _saveCacheToDiskQueued;
} while (counter1 != counter2);
Serialize(s_cacheStoreLocation);
Expand Down
4 changes: 1 addition & 3 deletions src/System.Management.Automation/engine/SessionStatePublic.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,7 @@ public static void ThrowIfNotVisible(CommandOrigin origin, object valueToCheck)
CommandInfo cinfo = valueToCheck as CommandInfo;
if (cinfo != null)
{
string commandName = null;
if (cinfo != null)
commandName = cinfo.Name;
string commandName = cinfo.Name;
if (commandName != null)
{
// If we have a name, use it in the error message
Expand Down
6 changes: 1 addition & 5 deletions src/System.Management.Automation/engine/hostifaces/PowerShell.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4432,11 +4432,7 @@ private void CoreInvoke<TInput, TOutput>(PSDataCollection<TInput> input, PSDataC
finally
{
RunningExtraCommands = false;

if (_isBatching)
{
EndAsyncBatchExecution();
}
EndAsyncBatchExecution();
}
}
else
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/interpreter/CallInstruction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ private static Type TryGetParameterOrReturnType(MethodInfo target, ParameterInfo

private static bool IndexIsNotReturnType(int index, MethodInfo target, ParameterInfo[] pi)
{
return pi.Length != index || (pi.Length == index && !target.IsStatic);
return pi.Length != index || !target.IsStatic;
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/interpreter/ControlFlowInstructions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ public override int Run(InterpretedFrame frame)
// In the second path, the continuation mechanism is not involved and frame.InstructionIndex is not updated
#if DEBUG
bool isFromJump = frame.IsJumpHappened();
Debug.Assert(!isFromJump || (isFromJump && _tryHandler.FinallyStartIndex == frame.InstructionIndex), "we should already jump to the first instruction of the finally");
Debug.Assert(!isFromJump || _tryHandler.FinallyStartIndex == frame.InstructionIndex, "we should already jump to the first instruction of the finally");
#endif
// run the finally block
// we cannot jump out of the finally block, and we cannot have an immediate rethrow in it
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/interpreter/InterpretedFrame.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ public int Goto(int labelIndex, object value, bool gotoExceptionHandler)
{
// TODO: we know this at compile time (except for compiled loop):
RuntimeLabel target = Interpreter._labels[labelIndex];
Debug.Assert(!gotoExceptionHandler || (gotoExceptionHandler && _continuationIndex == target.ContinuationStackDepth),
Debug.Assert(!gotoExceptionHandler || _continuationIndex == target.ContinuationStackDepth,
"When it's time to jump to the exception handler, all previous finally blocks should already be processed");

if (_continuationIndex == target.ContinuationStackDepth)
Expand Down
4 changes: 1 addition & 3 deletions src/System.Management.Automation/engine/remoting/client/ClientMethodExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,7 @@ internal static void Dispatch(
// Cmdlet case: queue up the executor in the pipeline stream.
if (isMethodExecutorStreamEnabled)
{
Dbg.Assert(!isMethodExecutorStreamEnabled ||
(isMethodExecutorStreamEnabled && methodExecutorStream != null),
"method executor stream can't be null when enabled");
Dbg.Assert(methodExecutorStream != null, "method executor stream can't be null when enabled");
methodExecutorStream.Write(methodExecutor);
}

Expand Down
7 changes: 2 additions & 5 deletions src/System.Management.Automation/engine/remoting/client/PSProxyJob.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,8 +769,7 @@ private static ICollection<PSJobProxy> Create(Runspace runspace, RunspacePool ru

using (PowerShell ps = PowerShell.Create())
{
Dbg.Assert(runspacePool == null && runspace != null ||
runspace == null && runspacePool != null, "Either a runspace or a runspacepool should be used, not both.");
Dbg.Assert(runspacePool == null ^ runspace == null, "Either a runspace or a runspacepool should be used, not both.");

if (runspacePool == null)
{
Expand Down Expand Up @@ -1493,9 +1492,7 @@ private void CleanupReceivePowerShell(IAsyncResult asyncResult)
/// happen</param>
private void AssignRunspaceOrRunspacePool(PowerShell powershell)
{
Dbg.Assert(_runspacePool == null && _runspace != null ||
_runspace == null && _runspacePool != null,
"Either a runspace or a runspacepool should be assigned to the proxy job");
Dbg.Assert(_runspacePool == null ^ _runspace == null, "Either a runspace or a runspacepool should be assigned to the proxy job");

if (_runspacePool == null)
{
Expand Down
3 changes: 1 addition & 2 deletions src/System.Management.Automation/engine/remoting/client/remotepipeline.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,8 +912,7 @@ private void InitPowerShellForConnect(bool syncCall)

// Check to see if this pipeline already exists in the runspace.
RemotePipeline currentPipeline = (RemotePipeline)((RemoteRunspace)_runspace).GetCurrentlyRunningPipeline();
if (currentPipeline == null ||
currentPipeline != null && !ReferenceEquals(currentPipeline, this))
if (!ReferenceEquals(currentPipeline, this))
{
((RemoteRunspace)_runspace).DoConcurrentCheckAndAddToRunningPipelines(this, syncCall);
}
Expand Down
11 changes: 3 additions & 8 deletions src/System.Management.Automation/engine/remoting/commands/InvokeCommandCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,10 +1173,7 @@ protected override void EndProcessing()
WriteJobResults(false);

// finally dispose the job.
if (!_asjob)
{
_job.Dispose();
}
_job.Dispose();

// We no longer need to call ClearInvokeCommandOnRunspaces() here because
// this command might finish before the foreach block finishes. previously,
Expand Down Expand Up @@ -1223,10 +1220,8 @@ protected override void EndProcessing()
WriteJobResults(false);

// finally dispose the job.
if (!_asjob)
{
_job.Dispose();
}
_job.Dispose();

} // if (needToCollect...
}// else - job == null
}
Expand Down
5 changes: 0 additions & 5 deletions src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1212,11 +1212,6 @@ internal void ConstructUri(String scheme, String computerName, Nullable<Int32> p
PortSetting = -1;
UseDefaultWSManPort = true;
}
else if (port.Value == DefaultPortHttp || port.Value == DefaultPortHttps)
{
PortSetting = port.Value;
UseDefaultWSManPort = false;
}
else
{
PortSetting = port.Value;
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/security/Authenticode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,7 @@ private static Signature GetSignatureFromWintrustData(
}
}

Diagnostics.Assert(((error == 0) && (signature != null)) || (error != 0), "GetSignatureFromWintrustData: general crypto failure");
Diagnostics.Assert(error != 0 || signature != null, "GetSignatureFromWintrustData: general crypto failure");

if ((signature == null) && (error != 0))
{
Expand Down