Multiple improvements by CodeRush Static Analysis #5132

Himura2la · 2017-10-16T14:37:36Z

msftclas · 2017-10-16T14:37:44Z

PaulHigin

PaulHigin · 2017-10-16T17:48:56Z

src/Microsoft.PowerShell.CoreCLR.Eventing/DotNetCode/Eventing/Reader/EventLogReader.cs

                    else
                    {
-                        //if inside the current buffer, we still cannot read the events, as the handles.
+                        //if inside the current buffer (_currentIndex + offset >= 0), we still cannot read the events, as the handles.


This comment still doesn't make sense to me. Maybe just remove it?

OK, removing the whole comment

PaulHigin · 2017-10-16T18:05:07Z

src/Microsoft.PowerShell.PSReadLine/Words.cs

-                }
-                i += 1;
-            }
+            for (; i < _singleton._buffer.Length && !InWord(i, wordDelimiters); i++) ;


I feel this is easier to read (with the additional parentheses):
for (; (i < _singleton._buffer.Length) && !InWord(I, wordDelimiters); I++);

Submit a PR to https://github.com/lzybkr/PSReadLine for any changes in the PSReadLine directory.

And I'll likely reject the change - In general I'm not a fan of empty loop bodies.

I reverted the file, this change is a anyway just a preference. Maybe I'll try to push it to the correct repository, thanks for the link.

PaulHigin · 2017-10-16T18:16:26Z

src/System.Management.Automation/engine/DataStoreAdapter.cs

-                    // Since drive1 is null it is less than drive2 which is not null
-                    return false;
-                }
+                // Since both drives are null, they are equal


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

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

PaulHigin · 2017-10-16T18:18:57Z

src/System.Management.Automation/engine/DataStoreAdapterProvider.cs

-            {
-                throw PSTraceSource.NewArgumentException("name");
-            }
+            // FIXME: Other checks?


Please remove this comment and verify that helpfile, description, psSnapIn parameters can be null.

Sorry, I think I didn't catch whether I should ensure the helpfile, description and psSnapIn are not null or just remove the comment.

@Himura2la Can you either remove this comment or file an issue and update the comment to reference the issue?

PaulHigin · 2017-10-16T18:22:31Z

src/System.Management.Automation/engine/Modules/AnalysisCache.cs

                    // writing the cache out in a timely matter isn't too important
                    // now anyway.
-                    await Task.Delay(10000);
+                    await Task.Delay(10000).ConfigureAwait(false);


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

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

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

PaulHigin · 2017-10-16T18:38:19Z

src/System.Management.Automation/security/Authenticode.cs

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

-            if ((signature == null) && (error != 0))
+            if (signature == null && error != 0)


I feel the code is more readable with parentheses back in. I am not sure what our coding guidelines say.

In this area, coding guidelines say - be consistent, but we also ask for no formatting changes mixed with other changes.

In reply to: 144931951 [](ancestors = 144931951)

I reverted this.
Sorry, I should have kept myself from removing parentheses everywhere. I think I write too much Python

lzybkr · 2017-10-16T19:09:07Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostRawUserInterface.cs

                            {
-                                Dbg.Assert((inputRecords[0].KeyEvent.KeyDown && inputRecords[0].KeyEvent.RepeatCount != 0) ||
-                                    !inputRecords[0].KeyEvent.KeyDown,
+                                Dbg.Assert(inputRecords[0].KeyEvent.RepeatCount != 0 || !inputRecords[0].KeyEvent.KeyDown,


This changes the order of evaluation - probably safe here, but before, RepeatCount would not be evaluated if KeyDown was false.

I swapped them, thanks for noticing. Although it seems really safe assuming there's even no null-checks

lzybkr · 2017-10-16T19:09:35Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostRawUserInterface.cs

                    {
-                        Dbg.Assert((inputRecords[i].KeyEvent.KeyDown && inputRecords[i].KeyEvent.RepeatCount != 0) ||
-                            !inputRecords[i].KeyEvent.KeyDown,
+                        Dbg.Assert(inputRecords[i].KeyEvent.RepeatCount != 0 || !inputRecords[i].KeyEvent.KeyDown,


This changes the order of evaluation - probably safe here, but before, RepeatCount would not be evaluated if KeyDown was false.

lzybkr · 2017-10-16T19:16:17Z

src/Microsoft.WSMan.Management/WsManHelper.cs

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


This comment can probably be deleted.

OK, I also inlined the tmpNs variable. I hope it's OK

lzybkr · 2017-10-16T19:46:32Z

src/System.Management.Automation/engine/remoting/client/remotepipeline.cs

            RemotePipeline currentPipeline = (RemotePipeline)((RemoteRunspace)_runspace).GetCurrentlyRunningPipeline();
-            if (currentPipeline == null ||
-                currentPipeline != null && !ReferenceEquals(currentPipeline, this))
+            if (currentPipeline == null || !ReferenceEquals(currentPipeline, this))


I think currentPipeline == null is redundant with ReferenceEquals here.

Seems like.

TravisEz13 · 2017-10-16T22:37:42Z

Himura2la · 2017-10-17T12:09:37Z

TravisEz13 · 2017-10-17T17:58:18Z

PaulHigin

TravisEz13 · 2017-10-18T17:42:39Z

src/System.Management.Automation/engine/DataStoreAdapterProvider.cs

-            {
-                throw PSTraceSource.NewArgumentException("name");
-            }
+            // FIXME: Other checks?


@Himura2la Can you either remove this comment or file an issue and update the comment to reference the issue?

TravisEz13 · 2017-10-18T17:52:47Z

Himura2la · 2017-10-18T18:14:58Z

TravisEz13 · 2017-10-18T19:10:17Z

iSazonov · 2017-11-08T12:47:05Z

Himura2la · 2017-11-08T15:58:46Z

SteveL-MSFT · 2017-11-08T18:06:02Z

Multiple improvements by CodeRush Static Analysis

b829325

Himura2la requested review from BrucePay, PaulHigin, adityapatwardhan, anmenaga, charub, dantraMSFT, daxian-dbw, lzybkr, mirichmo and vors as code owners October 16, 2017 14:37

daxian-dbw assigned TravisEz13 Oct 16, 2017

PaulHigin suggested changes Oct 16, 2017

View reviewed changes

lzybkr reviewed Oct 16, 2017

View reviewed changes

Changes to PR from the Team

4effc48

lzybkr approved these changes Oct 17, 2017

View reviewed changes

PaulHigin approved these changes Oct 17, 2017

View reviewed changes

TravisEz13 requested changes Oct 18, 2017

View reviewed changes

TravisEz13 approved these changes Oct 18, 2017

View reviewed changes

TravisEz13 merged commit 6530b77 into PowerShell:master Nov 10, 2017

Multiple improvements by CodeRush Static Analysis #5132

Multiple improvements by CodeRush Static Analysis #5132

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh o 10BC0 h!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels