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

Clean up code #4

Closed
wants to merge 1 commit into from
Closed

Clean up code #4

wants to merge 1 commit into from

Conversation

eltang
Copy link

@eltang eltang commented May 11, 2016

The code should be significantly easier to understand now.

uint8_t row = number / 8;
uint8_t mask = 1U << number % 8;
for (int8_t column = MAX_LAYER_BITS - 1; column >= 0; --column) {
mask_byte(&layers_cache[row][column], layer & _BV(column), mask);
Copy link
Owner

Choose a reason for hiding this comment

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

_BV seems to be defined in avr/io.h which is not included currently.

Copy link
Author

Choose a reason for hiding this comment

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

The code compiles for me. It seems that it's included in pgmspace.h, which is itself included in action_macro.h, which is itself included in action_layer.c.

Copy link
Owner

@vifon vifon May 11, 2016

Choose a reason for hiding this comment

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

It builds but I get a warning about an implicit function declaration from RTags. I'd rather stay on the safe side and include this header explicitly.

I still haven't tested if it works.

@vifon
Copy link
Owner

vifon commented May 11, 2016

Ok, I've tested it. Seems to be working. Apart from these two comments above, the code look good to me. Nonetheless it's a mostly opinion based change so let's see what the upstream will say. Submit it to Jack's repo and let's see. Can I close this PR or do you want me to merge it to my fork anyway?

@eltang
Copy link
Author

eltang commented May 12, 2016

I'll send a PR to the main repo.

@eltang eltang closed this May 12, 2016
@eltang eltang deleted the layers_cache_fix branch May 12, 2016 05:08
@vifon
Copy link
Owner

vifon commented May 12, 2016

Sorry, sleep etc. ;)

Regarding the inline functions: In this case it definitely won't hurt though the compiler might inline them anyway. Or might not anyway. inline is just a hint to the compiler.

@eltang
Copy link
Author

eltang commented May 12, 2016

It's jackhumbert#321.

vifon pushed a commit that referenced this pull request Oct 9, 2016
Added diagonal mice macros, breathing ala Atomic
vifon pushed a commit that referenced this pull request Oct 9, 2016
vifon pushed a commit that referenced this pull request May 3, 2017
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.

2 participants