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

Make loop unrolling completely whitespace-insensitive #20163

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

yushijinhun
Copy link
Contributor

This is just another #19726. Since #20012 is accepted, I think it's okay to make loop unrolling completely whitespace-insensitive.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 24, 2020

TBH, I'm not a fan of salami tactics. My opinion still stands:

#19726 (comment)

@mrdoob
Copy link
Owner

mrdoob commented Aug 24, 2020

I'm undecided...
@yushijinhun how does your code look like?

@yushijinhun
Copy link
Contributor Author

I'm undecided...
@yushijinhun how does your code look like?

I'm removing all unnecessary whitespaces from GLSL code to reduce bundle size.
So I hope the following style can be accepted:

#pragma unroll_loop_start
for(int i=0;i<5;i++){k+=m[i];}
#pragma unroll_loop_end

@mrdoob
Copy link
Owner

mrdoob commented Aug 25, 2020

Seems like there is no need to add \s+ to #pragmas?

This is just another mrdoob#19726.
Since mrdoob#20012 is accepted, I think it's okay to make loop unrolling
completely whitespace-insensitive.
@yushijinhun yushijinhun force-pushed the unroll-loops-whitespace-2 branch from e6cb904 to dc985f7 Compare August 25, 2020 11:06
@yushijinhun
Copy link
Contributor Author

Seems like there is no need to add \s+ to #pragmas?

Not quite necessary. I have removed them.

@mrdoob mrdoob added this to the r120 milestone Aug 25, 2020
@mrdoob mrdoob merged commit 0c57e2f into mrdoob:dev Aug 25, 2020
@mrdoob
Copy link
Owner

mrdoob commented Aug 25, 2020

Thanks!

@yushijinhun yushijinhun deleted the unroll-loops-whitespace-2 branch August 27, 2020 14:08
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.

3 participants