Skip to content
This repository has been archived by the owner on Dec 18, 2019. It is now read-only.

Literal: Remove null and boolean parsers #104

Merged
merged 3 commits into from
Sep 14, 2017
Merged

Literal: Remove null and boolean parsers #104

merged 3 commits into from
Sep 14, 2017

Conversation

Acconut
Copy link
Contributor

@Acconut Acconut commented May 22, 2017

Addresses #93.

@coveralls
Copy link

coveralls commented May 22, 2017

Coverage Status

Coverage decreased (-0.1%) to 93.4% when pulling 02f0009 on Acconut:remove_null_boolean_parser into e7aa963 on tagua-vm:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 93.4% when pulling 02f0009 on Acconut:remove_null_boolean_parser into e7aa963 on tagua-vm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 93.4% when pulling 02f0009 on Acconut:remove_null_boolean_parser into e7aa963 on tagua-vm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 93.4% when pulling 02f0009 on Acconut:remove_null_boolean_parser into e7aa963 on tagua-vm:master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 93.4% when pulling 02f0009 on Acconut:remove_null_boolean_parser into e7aa963 on tagua-vm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 93.4% when pulling 02f0009 on Acconut:remove_null_boolean_parser into e7aa963 on tagua-vm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 93.4% when pulling 02f0009 on Acconut:remove_null_boolean_parser into e7aa963 on tagua-vm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 93.4% when pulling 02f0009 on Acconut:remove_null_boolean_parser into e7aa963 on tagua-vm:master.

@@ -722,78 +640,6 @@ mod tests {
};

#[test]
fn case_null() {
let input = Span::new(b"null");
Copy link
Member

Choose a reason for hiding this comment

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

You should move these tests where constants are used, so in qualified_name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should all of these tests be moved? I am not sure if they are just redundant then, in particular since a test for unqualified names already exists:

#[test]
fn case_unqualified_name() {
let input = Span::new(b"Foo");
let output = Result::Done(Span::new_at(b"", 3, 1, 4), Name::Unqualified(input));
assert_eq!(qualified_name(input), output);
}

Copy link
Member

@Hywan Hywan May 29, 2017

Choose a reason for hiding this comment

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

I guess it's better to ensure that special constants are correctly parsed ;-). So I prefer to keep them.

@Acconut
Copy link
Contributor Author

Acconut commented May 23, 2017

@Hywan Do you have an idea why suddenly the tests for Rust 1.14 failed? https://travis-ci.org/tagua-vm/parser/jobs/234969027

@Hywan
Copy link
Member

Hywan commented May 23, 2017

A dependency is breaking our build. Continue, I will fix it on my end.

@Acconut
Copy link
Contributor Author

Acconut commented May 23, 2017

@Hywan Did you seen my response to your comment on the code? #104 (comment) :)

@Hywan
Copy link
Member

Hywan commented May 29, 2017

@Acconut Done.

@coveralls
Copy link

coveralls commented May 31, 2017

Coverage Status

Coverage decreased (-0.08%) to 93.426% when pulling e7acb9b on Acconut:remove_null_boolean_parser into e7aa963 on tagua-vm:master.

@Acconut
Copy link
Contributor Author

Acconut commented May 31, 2017

Added back the tests.

@Hywan
Copy link
Member

Hywan commented Jun 6, 2017

Is it ready for a review?

@Acconut
Copy link
Contributor Author

Acconut commented Jun 6, 2017

Yes, indeed :)

@Hywan
Copy link
Member

Hywan commented Jun 7, 2017

I have a bad news for you. Yes, according to the grammar, true, false, and null are not constants but qualified names. Yes, we should remove these parsers. But, I just remembered these tweets, https://twitter.com/hoaproject/status/688991362151280640 and https://twitter.com/hoaproject/status/689459255485698049. Quoting (and simplifying):

@hoaproject: https://3v4l.org/Lttus is it a regression?
@nikic: No, it's an intentional change. true/false/null are always off-limits now, while previously only sometimes.
@hoaproject: Why are they off-limit now :-)?
@nikic: Because they are really language items that happen to be implemented through constants. It should not be possible to overload em

So, as far as I understand, they are qualified names with special treatements. Thus, my conclusion would be to keep them right now, and see if it raises some errors in existing test suites from Zend or HHVM.

What do you think?

I am sorry it comes to my mind riht now after all your work. Maybe an input from @nikic or @jubianchi would confirm.

@Acconut
Copy link
Contributor Author

Acconut commented Jun 10, 2017

That's interesting, I wasn't aware of this fact. And you don't have to apologize, it's better to think of this now than later :)

Another option, I could think of is treat them similar to how magic constants, such as LINE may be implemented (see http://php.net/manual/en/language.constants.predefined.php). They could be parsed as qualified names but have a special treatment.

@nikic
Copy link

nikic commented Jun 10, 2017

@Hywan Haven't followed the context here, but please note that while true, false and null are excluded from the namespace fallback, they still generally participate in name resolution. For example writing use const false as true; is valid and will make true resolve to false.

@Hywan Hywan merged commit e7acb9b into tagua-vm:master Sep 14, 2017
@Hywan
Copy link
Member

Hywan commented Sep 14, 2017

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants