-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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.
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
Conversation
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.
Thanks for finding and fixing these!
| 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. |
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.
This comment still doesn't make sense to me. Maybe just remove it?
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.
OK, removing the whole comment
| } | ||
| i += 1; | ||
| } | ||
| for (; i < _singleton._buffer.Length && !InWord(i, wordDelimiters); i++) ; |
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 feel this is easier to read (with the additional parentheses):
for (; (i < _singleton._buffer.Length) && !InWord(I, wordDelimiters); I++);
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.
Submit a PR to https://github.com/lzybkr/PSReadLine for any changes in the PSReadLine directory.
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.
And I'll likely reject the change - In general I'm not a fan of empty loop bodies.
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 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.
| // Since drive1 is null it is less than drive2 which is not null | ||
| return false; | ||
| } | ||
| // Since both drives are null, they are equal |
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.
This doesn't look right. I believe it should be.
return (drive2Object != 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.
Oh, thanks, I didn't notice that the refactoring can go even further
| { | ||
| throw PSTraceSource.NewArgumentException("name"); | ||
| } | ||
| // FIXME: Other checks? |
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.
Please remove this comment and verify that helpfile, description, psSnapIn parameters can be 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.
Sorry, I think I didn't catch whether I should ensure the helpfile, description and psSnapIn are not null or just remove the comment.
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.
@Himura2la Can you either remove this comment or file an issue and update the comment to reference the issue?
| // writing the cache out in a timely matter isn't too important | ||
| // now anyway. | ||
| await Task.Delay(10000); | ||
| await Task.Delay(10000).ConfigureAwait(false); |
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.
It is not clear to me that this is needed. Isn't the Task always configured as "false" be default?
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.
It seems right though, so I think it's good to keep it.
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.
@PaulHigin As far as I know, the default option is 'true'. And that's the reason for adding an explicit ConfigureAwait() everywhere.
| Diagnostics.Assert(error != 0 || signature != null, "GetSignatureFromWintrustData: general crypto failure"); | ||
|
|
||
| if ((signature == null) && (error != 0)) | ||
| if (signature == null && error != 0) |
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 feel the code is more readable with parentheses back in. I am not sure what our coding guidelines say.
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.
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)
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 reverted this.
Sorry, I should have kept myself from removing parentheses everywhere. I think I write too much Python
| { | ||
| 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, |
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.
This changes the order of evaluation - probably safe here, but before, RepeatCount would not be evaluated if KeyDown was false.
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 swapped them, thanks for noticing. Although it seems really safe assuming there's even no null-checks
| { | ||
| 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, |
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.
This changes the order of evaluation - probably safe here, but before, RepeatCount would not be evaluated if KeyDown was false.
| } | ||
| //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. |
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.
This comment can probably be deleted.
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.
OK, I also inlined the tmpNs variable. I hope it's OK
| RemotePipeline currentPipeline = (RemotePipeline)((RemoteRunspace)_runspace).GetCurrentlyRunningPipeline(); | ||
| if (currentPipeline == null || | ||
| currentPipeline != null && !ReferenceEquals(currentPipeline, this)) | ||
| if (currentPipeline == null || !ReferenceEquals(currentPipeline, this)) |
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 currentPipeline == null is redundant with ReferenceEquals here.
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.
Seems like.
|
@Himura2la What is your status on signing the CLA? |
|
I'm done with the changes and CLA. The only thing left is extra checks here |
|
@PaulHigin Just a reminder to update your review. Restarted macOS due to random brew download failure. |
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
| { | ||
| throw PSTraceSource.NewArgumentException("name"); | ||
| } | ||
| // FIXME: Other checks? |
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.
@Himura2la Can you either remove this comment or file an issue and update the comment to reference the issue?
|
I'm following up on one issue. Hopefully there will be no more delays once I have the answer. |
|
@TravisEz13, I've removed the comment in my second commit |
|
@Himura2la Yeah, that is not the issue. |
|
Is the PR ready to merge? |
|
I wonder |
|
@TravisEz13 is this ready to merge? |
Hi guys!
I have checked the PowerShell solution with the CodeRush Static Analyzer and fixed several code issues. Here's the summary:
whileloops caught my attention. They look very similar to nested conditionals and I decided to fold them into for loops without bodies, they look clearer in this form.ConfigureAwait()call toawaited calls.I hope this will help, here's the full report: CodeIssuesReport.txt. Thanks for being open source!