-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Static Code Analysis with Php Inspections (EA Extended) #6121
Conversation
@@ -180,6 +180,7 @@ public function convertDoctrine1Schema(array $fromPaths, $destPath, $toType, $nu | |||
); | |||
} | |||
} | |||
unset($dirName); |
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.
Not sure if I like this inspection. $dirName
is not used after this location: let the stack garbage collection do its job.
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's more tricky: https://3v4l.org/QKOKH - here unset should clean the scope after foreach.
UPD: relevant only to foreach-values being a reference
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 know why this is applied, it just doesn't make sense if the variables are unused. I'd rather prefer extracting the loop into a private method instead.
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 see, good approach btw. Reverting the change.
@@ -1629,6 +1629,7 @@ protected function _validateAndCompleteOneToOneMapping(array $mapping) | |||
? $joinColumn['fieldName'] | |||
: $joinColumn['name']; | |||
} | |||
unset($joinColumn); |
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.
Same as below: this inspection is (IMO) not a good idea.
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.
Reverting. I also curious why it's not a good idea, why?
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's just an additional instruction (also to read - not worried about performance)
@Ocramius : I reverted usset constructs here, but checks are still saying that you requested changes. |
@kalessil thanks for this, merging! |
Couple of findings