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

Fix probable boolean logic error #13200

Merged
merged 1 commit into from Dec 28, 2017

Conversation

Projects
None yet
3 participants
@mmcco
Contributor

mmcco commented Dec 8, 2017

  • Type: bug fix
  • I have read and understood the Contributing Guidelines?
  • I have checked that another pull request for this purpose does not exist.
  • I wrote some tests for this PR.

Small description of change: This if condition is always false, and it seems clear that the author meant to use || rather than &&.

Thanks for your time,
Michael McConville
University of Utah

@mmcco mmcco changed the title from Probable boolean logic error to Fix probable boolean logic error Dec 8, 2017

@mmcco mmcco referenced this pull request Dec 8, 2017

Closed

Fix probable boolean logic error #13198

2 of 2 tasks complete
@Ultimater

This comment has been minimized.

Show comment
Hide comment
@Ultimater

Ultimater Dec 8, 2017

Member

Good eye! And thanks for repairing the test!

Interestingly enough, this change wouldn't affect the CSS minimizing logic whatsoever without a break; as the STATE_FREE and STATE_SELECTOR states currently have no differences and would still encounter:

p = cssmin_back_peek(parser);
if (p == ',' || p == '>' || p == ':') {
     c = 0;
} else {
     c = ' ';
}

In other words we could actually write:

case STATE_FREE:
case STATE_SELECTOR:

and it would be the equivalent of either before or after your change.

I think what the author intended was actually to trim ALL whitespace when in the STATE_FREE, without using looking around to make sure we're not in a selector:

if (c == ' ' || c == '\t' || c == '\n' || c == '\r') {
    c = 0;
    break;
}

Note the usage of my break.

Preserving the space is necessary on the selector for example:
div p{margin:2px 4px 5px 7px;}
However if we're in the STATE_FREE, we have not yet encountered the first character d of div, so it's safe to remove all whitespace until such a character is encountered.

While we're in here, it would be nice to change:
parser.state = 1; to parser.state = STATE_FREE;
This way it's easier to search for STATE_FREE and find where it's initially set.

Also last_state doesn't appear to be used whatsoever and would be nice to trash.

Probably some room to fix the parser code further as mentioned at the top of the file:

** cannot handle nested { blocks but will ignore aditional { in parens ()
** no in quote detection for ( or }
Member

Ultimater commented Dec 8, 2017

Good eye! And thanks for repairing the test!

Interestingly enough, this change wouldn't affect the CSS minimizing logic whatsoever without a break; as the STATE_FREE and STATE_SELECTOR states currently have no differences and would still encounter:

p = cssmin_back_peek(parser);
if (p == ',' || p == '>' || p == ':') {
     c = 0;
} else {
     c = ' ';
}

In other words we could actually write:

case STATE_FREE:
case STATE_SELECTOR:

and it would be the equivalent of either before or after your change.

I think what the author intended was actually to trim ALL whitespace when in the STATE_FREE, without using looking around to make sure we're not in a selector:

if (c == ' ' || c == '\t' || c == '\n' || c == '\r') {
    c = 0;
    break;
}

Note the usage of my break.

Preserving the space is necessary on the selector for example:
div p{margin:2px 4px 5px 7px;}
However if we're in the STATE_FREE, we have not yet encountered the first character d of div, so it's safe to remove all whitespace until such a character is encountered.

While we're in here, it would be nice to change:
parser.state = 1; to parser.state = STATE_FREE;
This way it's easier to search for STATE_FREE and find where it's initially set.

Also last_state doesn't appear to be used whatsoever and would be nice to trash.

Probably some room to fix the parser code further as mentioned at the top of the file:

** cannot handle nested { blocks but will ignore aditional { in parens ()
** no in quote detection for ( or }
@Ultimater

This comment has been minimized.

Show comment
Hide comment
@Ultimater

Ultimater Dec 8, 2017

Member

Oh wait, a slight difference:

if ((c == ' ' || c == '\t')) {
    p = cssmin_peek(parser);
    if (p == '{' || p == '\t' || p == ' ' || p == '>' || p == ',') {
    c = 0;
} else {
    p = cssmin_back_peek(parser);
    if (p == ',' || p == '>' || p == ':') {
        c = 0;
    } else {
        c = ' ';
    }
}

c would be set to 0 in STATE_FREE of your version before it runs into the above code in STATE_SELECTOR.
Yeah, so that would explain the need to adjust the test when it was working before.

I would sooner still see a break; though. If we're changing c, it's spaghetti code if we let the states spill through, specially after changing an important variable holding the character we're working on...

Member

Ultimater commented Dec 8, 2017

Oh wait, a slight difference:

if ((c == ' ' || c == '\t')) {
    p = cssmin_peek(parser);
    if (p == '{' || p == '\t' || p == ' ' || p == '>' || p == ',') {
    c = 0;
} else {
    p = cssmin_back_peek(parser);
    if (p == ',' || p == '>' || p == ':') {
        c = 0;
    } else {
        c = ' ';
    }
}

c would be set to 0 in STATE_FREE of your version before it runs into the above code in STATE_SELECTOR.
Yeah, so that would explain the need to adjust the test when it was working before.

I would sooner still see a break; though. If we're changing c, it's spaghetti code if we let the states spill through, specially after changing an important variable holding the character we're working on...

@sergeyklay sergeyklay self-assigned this Dec 8, 2017

@sergeyklay sergeyklay added this to the 3.3.x milestone Dec 8, 2017

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Dec 21, 2017

Member

@mmcco Could you please update change log?

Member

sergeyklay commented Dec 21, 2017

@mmcco Could you please update change log?

sergeyklay added a commit that referenced this pull request Dec 24, 2017

@sergeyklay sergeyklay merged commit bc94406 into phalcon:3.3.x Dec 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Dec 28, 2017

Member

Thank you

Member

sergeyklay commented Dec 28, 2017

Thank you

sergeyklay added a commit that referenced this pull request Jan 8, 2018

chilimatic added a commit to chilimatic/cphalcon that referenced this pull request Jan 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment