bitset: Reimplement BIT_FOREACH_IS(SET|CLR)
ClosedPublic
Actions

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

Details

Reviewers

kib
cem
rlibby

Group Reviewers

manpages

Commits

rG51425cb2107c: bitset: Reimplement BIT_FOREACH_IS(SET|CLR)

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

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 42097
Build 38985: arc lint + arc unit

Event Timeline

markj created this revision.Oct 12 2021, 9:10 PM

Herald added a subscriber: imp. · View Herald TranscriptOct 12 2021, 9:10 PM

markj requested review of this revision.Oct 12 2021, 9:10 PM

Harbormaster completed remote builds in B42092: Diff 96765.Oct 12 2021, 9:10 PM

kib accepted this revision.Oct 12 2021, 11:31 PM

This revision is now accepted and ready to land.Oct 12 2021, 11:31 PM

rlibby accepted this revision.Oct 13 2021, 1:00 AM

rlibby added inline comments.

sys/sys/bitset.h
280	`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
70–73	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 marked an inline comment as done.Oct 13 2021, 1:32 AM

markj added inline comments.

sys/sys/bitset.h
280	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
70–73	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.

Parenthesize assignment, update man page.

This revision now requires review to proceed.Oct 13 2021, 1:36 AM

Herald added a reviewer: manpages. · View Herald TranscriptOct 13 2021, 1:36 AM

Harbormaster completed remote builds in B42097: Diff 96775.Oct 13 2021, 1:36 AM

rlibby accepted this revision.Oct 13 2021, 1:41 AM

rlibby added inline comments.

share/man/man9/bitset.9
363–364	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.Oct 13 2021, 1:41 AM

markj added inline comments.Oct 13 2021, 2:48 PM

share/man/man9/bitset.9
363–364	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.