Skip to content
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

[minor] SCA: minor code tweaks #15571

Merged
merged 1 commit into from
Jan 29, 2018
Merged

[minor] SCA: minor code tweaks #15571

merged 1 commit into from
Jan 29, 2018

Conversation

kalessil
Copy link
Contributor

Q A
Is bugfix? no
New feature? no
Breaks BC? no
Tests pass? no
Fixed issues n/a

@@ -874,7 +874,7 @@ protected function splitChangelog($file, $version)
$state = 'end';
}
// add continued lines to the last item to keep them together
if (!empty(${$state}) && trim($line !== '') && strpos($line, '- ') !== 0) {
if (!empty(${$state}) && trim($line) !== '' && strncmp($line, '- ', 2) !== 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This one looks serious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little bit, but due to weak typing this worked. I just noticed that we can drop trim($line) !== '' here at all, should I do it?

@@ -307,7 +307,7 @@ public function testBatchInsert()

public function testBatchInsertWithYield()
{
if (version_compare(PHP_VERSION, '5.5', '<')) {
if (PHP_VERSION_ID < 50500) {
Copy link
Member

Choose a reason for hiding this comment

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

Why? It's less readable.

Copy link
Contributor Author

@kalessil kalessil Jan 29, 2018

Choose a reason for hiding this comment

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

Optimizer friendly, in case of the constant it can drop the branch when generating opcodes. One thing I'm not sure if it should be prefixed with '\' for this or not. if I find the link where this described, I'll poke you at Twitter.

Copy link
Member

Choose a reason for hiding this comment

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

That's actually OK and makes sense but in this case it's up to code style (performance difference doesn't matter much).

@samdark samdark added this to the 2.0.14 milestone Jan 29, 2018
@samdark samdark self-assigned this Jan 29, 2018
@kalessil
Copy link
Contributor Author

@samdark : will it be welcome a PR covering this case https://github.com/symfony/symfony/pull/25854/files (it's 10-20 functions like that) ?

@samdark
Copy link
Member

samdark commented Jan 29, 2018

Yes.

@kalessil
Copy link
Contributor Author

Super, thank you. I'll create a separate one today (evening or so).

@samdark samdark merged commit 67f67e3 into yiisoft:master Jan 29, 2018
@samdark
Copy link
Member

samdark commented Jan 29, 2018

Thank you for code improvements.

@kalessil
Copy link
Contributor Author

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants