-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
Added support for keywords for class/method/label/declare/namespace/class const names #438
Conversation
((Z_STRLEN_P(name) == sizeof("parent")-1) && | ||
!memcmp(lcname, "parent", sizeof("parent")-1)) || | ||
((Z_STRLEN_P(name) == sizeof("static")-1) && | ||
!memcmp(lcname, "parent", sizeof("static")-1))) { |
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.
nit: should be static
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.
Hmpf, fixed. Had that already fixed locally, but forgotten to commit it.
Actually, having it in parser, also caused tokenizer and highlighter to return the keyword tokens instead of a T_STRING token which might confuse parsers (and lead this way to a BC break) Also this way we do not need to maintain a fixed list in parser which needs to be updated on every addition/modification of keywords Furthermore it allows more control over tokens before they are passed to parser (for future features...)
@@ -2,10 +2,12 @@ | |||
Bug #43343 (Variable class name) | |||
--FILE-- | |||
<?php | |||
namespace Foo; | |||
class Bar { } | |||
class if { |
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.
why replace a working test with something completely different? If we need to test for if as class name, we can add different test, but that's not what this test was about.
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.
@smalyshev actually a class named namespace
is supported. So the test was superfluous in it's old state.
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.
@bwoebi if it's supported, change the outcome of the test, why rewrite it to something completely different testing something else not related to the bug?
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.
@smalyshev good question… Don't remember why I had changed the test. Reverted test now and updated expectf to match actual state.
Comment on behalf of nikic at php.net: Closing as this RFC failed and there now is a new one. |
This patch adds possibility to use keywords at some places where previously were only T_STRING's allowed.
Example:
For further examples see the rfc at https://wiki.php.net/rfc/keywords_as_identifiers
The goal of the patch is to limit less the user's choice of names for labels, classes, functions, namespaces etc.
Internals discussion at http://marc.info/?l=php-internals&m=137893452326358&w=2
Possible constructs (with
string
being a T_STRING or a keyword):What still is impossible: (as function names and constants remain unaffected)