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

fail: incorrect conditional logic as else blocks move to new location #143

Closed
Troid92 opened this issue Aug 11, 2022 · 1 comment · Fixed by #150
Closed

fail: incorrect conditional logic as else blocks move to new location #143

Troid92 opened this issue Aug 11, 2022 · 1 comment · Fixed by #150

Comments

@Troid92
Copy link

Troid92 commented Aug 11, 2022

Input:

float getNum(float a) {
  if (a > 0.0) {
    if (a > 1.0) return 1.0;
  }
  else return 0.0;
  return a;
}

In the above, getNum(0.5) returns 0.5 and getNum(-1.0) returns 0.

Shader_Minifier output (text format):

float f(float f){if(f>0.)if(f>1.)return 1.;else return 0.;return f;}

In the above, f(0.5) returns 0. and f(-1.0) returns -1.0

Expected output:

float f(float f){if(f>0.){if(f>1.)return 1.;}else return 0.;return f;}

In the above, f(0.5) returns 0.5 and f(-1.0) returns 0.

While this example can be simplified to clamp(a,0.,1.), I did experience this bug in a situation where the logic could not be simplified.

Workarounds:
The best workaround I could find is to use an older version of Shader_Minifier as this is technically a regression.
Otherwise I am not aware of any catch-all solution. In the above example, the conditional logic could be flipped (start with if (a <= 0.0) return 0.0; else ...) which would produce the correct result. However, if the current else statement had similarly nested ifs then flipping the branches would no longer work. Adding an empty else {} in the inner block will not work as it will be removed by Shader_Minifier.

@laurentlb
Copy link
Owner

Thanks for the nice report!

This seems to be a corner-case where removing the curly braces isn't safe. I suppose we could preserve the curly braces when the inner block is a if.

I'm surprised older versions of the Minifier didn't have this bug. We'll need to investigate and see what happened.

laurentlb added a commit that referenced this issue Oct 8, 2022
Detect if the statement inside a `if` might accept a dangling else.
This can handle cases like:
     if(a) for(;;) if(b) {} else {}

Fixes #143
laurentlb added a commit that referenced this issue Oct 8, 2022
Detect if the statement inside a `if` might accept a dangling else.
This can handle cases like:
     if(a) for(;;) if(b) {} else {}

Fixes #143
laurentlb added a commit that referenced this issue Oct 9, 2022
Detect if the statement inside a `if` might accept a dangling else.
This can handle cases like:
     if(a) for(;;) if(b) {} else {}

Fixes #143
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 a pull request may close this issue.

2 participants