Skip to content

Get rid of unnecessary grouping operators - #6

Merged
dhritzkiv merged 11 commits into
stackgl:masterfrom
axion014:minify-groups
Jun 11, 2019
Merged

Get rid of unnecessary grouping operators#6
dhritzkiv merged 11 commits into
stackgl:masterfrom
axion014:minify-groups

Conversation

@axion014

@axion014 axion014 commented Jun 7, 2019

Copy link
Copy Markdown
Contributor

Closes #5.

@dhritzkiv

Copy link
Copy Markdown
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.

Comment thread 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
Copy Markdown
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

axion014 commented Jun 9, 2019

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

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

@dhritzkiv

Copy link
Copy Markdown
Member

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

@dhritzkiv

Copy link
Copy Markdown
Member

Btw, does - qualify as a commutative operator?

# Conflicts:
#	tap-snapshots/test-basic.js-TAP.test.js
#	test/basic.js
@axion014

Copy link
Copy Markdown
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
Copy Markdown
Member

Duh!

@dhritzkiv
dhritzkiv merged commit a284825 into stackgl:master Jun 11, 2019
@axion014

Copy link
Copy Markdown
Contributor Author

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

@dhritzkiv

Copy link
Copy Markdown
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