Skip to content

MutatingScope: implemented support for BitwiseNot operator#249

Closed
dktapps wants to merge 2 commits intophpstan:masterfrom
pmmp:bitwise-not
Closed

MutatingScope: implemented support for BitwiseNot operator#249
dktapps wants to merge 2 commits intophpstan:masterfrom
pmmp:bitwise-not

Conversation

@dktapps
Copy link
Copy Markdown
Contributor

@dktapps dktapps commented Jun 18, 2020

No description provided.

Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Best way to test this is to create a new dataProvider for NodeScopeResolverTest::testFileAsserts.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This if isn't necessary

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I assumed it was since other similar operators need it as well. I'll get rid of it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not covered cases:

  • floats
  • unions of constant strings

Best way to tackle this is to use TypeTraverse::map() which takes care of complex types easily...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Floats get coerced to integer for bitwise not. I hadn't considered the union types problem.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh right, floats work (but a unit test would be nice).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The changed rule should be in separate commit along with the related test changes.

@ondrejmirtes
Copy link
Copy Markdown
Member

Merged as 24246f3, thank you.

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