Skip to content

bitfield: get first set bit#3573

Closed
OlegHahm wants to merge 5 commits intoRIOT-OS:masterfrom
OlegHahm:bitfield_is_set
Closed

bitfield: get first set bit#3573
OlegHahm wants to merge 5 commits intoRIOT-OS:masterfrom
OlegHahm:bitfield_is_set

Conversation

@OlegHahm
Copy link
Copy Markdown
Member

@OlegHahm OlegHahm commented Aug 5, 2015

  • unittests

@OlegHahm OlegHahm added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Aug 5, 2015
miri64
miri64 reviewed Aug 5, 2015
View reviewed changes
tests/unittests/tests-bitfield/tests-bitfield.c
Copy link
Copy Markdown
Member

@miri64 miri64 Aug 5, 2015

Choose a reason for hiding this comment

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

Why not use the BITFIELD macro with BF_SIZE == 40 here? That would make the code below a little bit easier to read:

BITFIELD(field, BF_SIZE);

Copy link
Copy Markdown
Member Author

@OlegHahm OlegHahm Aug 5, 2015

Choose a reason for hiding this comment

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

I think the rational was to not rely on the BITFIELD macro here.

@OlegHahm OlegHahm mentioned this pull request Aug 5, 2015
Merged
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Aug 5, 2015

We should mention that for bitfields with at most 32bit there is a more efficient version in bitarithm.h.

@OlegHahm
Copy link
Copy Markdown
Member Author

OlegHahm commented Aug 5, 2015

We should mention that for bitfields with at most 32bit there is a more efficient version in bitarithm.h.

Agreed - though their feature set is a little bit different.

@OlegHahm
Copy link
Copy Markdown
Member Author

OlegHahm commented Aug 5, 2015

But maybe we could add the missing functionality to bitarithm. Shouldn't be too difficult (TM).

cgundogan
cgundogan reviewed Aug 6, 2015
View reviewed changes
sys/bitfield/bitfield.c
Copy link
Copy Markdown
Member

@cgundogan cgundogan Aug 6, 2015

Choose a reason for hiding this comment

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

skipping words until a positive match is found and then skipping bytes might converge faster

Copy link
Copy Markdown
Member Author

@OlegHahm OlegHahm Aug 6, 2015

Choose a reason for hiding this comment

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

We had this discussion when @kaspar030 introduced bf_get_unset and decided against it. I think it's mostly a question of what sizes for bitfields you expect and trading code space for performance.

Copy link
Copy Markdown
Contributor

@Kijewski Kijewski Aug 6, 2015

Choose a reason for hiding this comment

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

I guess memory alignment could also be a problem if you read words.

@OlegHahm
Copy link
Copy Markdown
Member Author

OlegHahm commented Aug 6, 2015

Updated. Anyone for an ACK?

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 6, 2015
@OlegHahm
Copy link
Copy Markdown
Member Author

OlegHahm commented Aug 7, 2015

Got rid of the need for this in #3561. Should I close this one or does anyone else see a need for it?

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Aug 7, 2015 via email

@OlegHahm OlegHahm closed this Aug 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@kaspar030 kaspar030

Labels

CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@OlegHahm @kaspar030 @jnohlgard @Kijewski @miri64 @cgundogan