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

Get rid of unnecessary grouping operators #6

Merged
merged 11 commits into from
Jun 11, 2019

Conversation

axion014
Copy link
Contributor

@axion014 axion014 commented Jun 7, 2019

Closes #5.

@dhritzkiv
Copy link
Member

Hey @axion014. Thanks for this and your other commits.

I just updated the repo to include tests. Do you suppose you could add a test in test/basic.js?

Feel free to add simpler .glsl files to the test directory that helps you test your new code. Also add some assertions that validate that no edge-cases or gotchas arise.

index.js Outdated
// remove unnecessary grouping operators
if(is_unnecessary_group(node)) return

if(node.parent) for(var current = node.parent; current.parent; current = current.parent) {
Copy link
Member

Choose a reason for hiding this comment

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

For stylistic/readability purposes, can you give this if statement braces, and encapsulate the for loop within it?

@axion014
Copy link
Contributor Author

axion014 commented Jun 9, 2019

I just found it tricky to get right. For a minifier I would want it to recognize when a operator is commutative.
However, the same * operator can be used both on scalar and matrix, which makes it difficult.

@dhritzkiv
Copy link
Member

Feel free to implement tests for #7 and #8 before coming back to this one

@dhritzkiv
Copy link
Member

I updated the tests here as well to be more readable + succinct

@dhritzkiv
Copy link
Member

Btw, does - qualify as a commutative operator?

# Conflicts:
#	tap-snapshots/test-basic.js-TAP.test.js
#	test/basic.js
@axion014
Copy link
Contributor Author

Btw, does - qualify as a commutative operator?

It doesn't - 1 - 2 !== 2 - 1

# Conflicts:
#	index.js
#	tap-snapshots/test-basic.js-TAP.test.js
#	test/basic.js
@dhritzkiv
Copy link
Member

Duh!

@dhritzkiv dhritzkiv merged commit a284825 into stackgl:master Jun 11, 2019
@axion014
Copy link
Contributor Author

Thanks for your time on maintaining this repository. You appear to be going even further, eh?

@dhritzkiv
Copy link
Member

@axion014 No problem. I'll push a release soon.

Your redundant vec parameters feature inspired me to apply it to mats! Mind taking a look and seeing if it makes sense and if it would be useful to you? #9

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.

Unnecessary parentheses
2 participants