Page MenuHomeFreeBSD

bitset: Reimplement BIT_FOREACH_IS(SET|CLR)
ClosedPublic

Authored by markj on Tue, Oct 12, 9:10 PM.

Details

Summary

Eliminate the nesting and re-implement following a suggestion from
rlibby.

It does seem to be possible to implement the macro using nested loops in
a way that lets "break" work, but it's kind of ugly and gcc -O2 at least
generates pessimal code with that implementation; clang manages to
generate slightly faster code with that approach vs this one, but it
doesn't seem worth it.

Add some simple regression tests.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Tue, Oct 12, 9:10 PM
This revision is now accepted and ready to land.Tue, Oct 12, 11:31 PM
rlibby added inline comments.
sys/sys/bitset.h
279

i is passed through from the caller, don't we want it parenthesized? Or do we make an exception next to an assignment operator, since there isn't really a sensible lower precedence operator that might be used in i?

tests/sys/sys/bitset_test.c
69–72

Hmm, should we mention this in the doc? If you modify a bitset while you're iterating it, you might not see that modification reflected in the iteration (if it's a later bit in the same word). I think this is acceptable but it is a potential gotcha.

Alternately, we could make it work by reloading the word and doing some shifting, but it would probably be slower for all users and most code would probably not do such a thing.

markj added inline comments.
sys/sys/bitset.h
279

I couldn't imagine a scenario where parens were needed to give correct behaviour here. But it is probably better to be consistent.

tests/sys/sys/bitset_test.c
69–72

I will add a note to the manual page. I think the current behaviour is sane though. queue.h macros have some similar constraints: for example, one cannot insert an item following the current item in a TAILQ_FOREACH_SAFE traversal and expect to visit the new item during the next iteration.

markj marked an inline comment as done.

Parenthesize assignment, update man page.

This revision now requires review to proceed.Wed, Oct 13, 1:36 AM
rlibby added inline comments.
share/man/man9/bitset.9
362–363

So, as coded, I think this would be "in the current iteration" -- they should be reflected in later iterations after this one. However if you just prefer this to be more general, that's fine too.

This revision is now accepted and ready to land.Wed, Oct 13, 1:41 AM
share/man/man9/bitset.9
362–363

By "in the current iteration" I meant a single execution of the loop body, not a full iteration over the set. I tried to make this a bit more clear.

This revision now requires review to proceed.Wed, Oct 13, 2:48 PM
rlibby added inline comments.
share/man/man9/bitset.9
362–363

Ah, yeah, thanks for the clarification.

This revision is now accepted and ready to land.Wed, Oct 13, 4:43 PM
This revision was automatically updated to reflect the committed changes.