Skip to content

Comments

bit testing and setting functions#4703

Closed
burner wants to merge 1 commit intodlang:masterfrom
burner:bit_testing_and_setting_functions
Closed

bit testing and setting functions#4703
burner wants to merge 1 commit intodlang:masterfrom
burner:bit_testing_and_setting_functions

Conversation

@burner
Copy link
Member

@burner burner commented Aug 2, 2016

Bit Testing Functions

bool testBit(T)(const T bitfield, const ulong idx) if(isIntegral!T);

bool testAnyBit(T)(const T bitfield) if(isIntegral!T);

bool testNoBit(T)(const T bitfield) if(isIntegral!T);

bool testAllBit(T)(const T bitfield) if(isIntegral!T);

Bit Setting Functions

T setBit(T)(const T bitfield, const ulong idx) if(isIntegral!T);

T setBit(T)(const T bitfield, const ulong idx, bool value) if(isIntegral!T);

T flipBit(T)(const T bitfield, const ulong idx) if(isIntegral!T);

T resetBit(T)(const T bitfield, const ulong idx) if(isIntegral!T);

Bit Pretty Printing

void bitsPrettyPrint(T,Out)(const T value, ref Out output)
    if (isIntegral!T && isOutputRange!(Out,string));

string bitsPrettyPrint(T)(const T value) if (isIntegral!T);

github: https://github.com/burner/BitFiddle
dub: http://code.dlang.org/packages/bitfiddle

@burner
Copy link
Member Author

burner commented Aug 2, 2016

These are very short functions but getting them right thanks to value range propagation is a pain. Now you just have to call a function.

@burner burner force-pushed the bit_testing_and_setting_functions branch from 0f9d6e5 to 9fcecd9 Compare August 2, 2016 13:23
@codecov-io
Copy link

Current coverage is 88.73% (diff: 100%)

No coverage report found for master at 7b30453.

Powered by Codecov. Last update 7b30453...9fcecd9

@dnadlinger
Copy link
Contributor

So what is it that makes just writing out the operations "a pain"?

I'm not convinced that extra functions on this level are needed (low-level bit twiddling for those who know what they are doing, and logical higher-level bit fields, ... for a more user-friendly API).

@burner
Copy link
Member Author

burner commented Aug 2, 2016

@klickverbot it inserts the right casts at the right spots so you don't have to remember that byte become ints when you bitwise (or|and) them. of cause some people know, but calling one of those functions is way easier.

@9il
Copy link
Member

9il commented Aug 2, 2016

Maybe Bit Setting Functions is enough?

assert(testBit(a, 0));
assert(!testBit(a, 1));
assert(testBit(a, 5));
assert(!testBit(a, 6));
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs tests for more types.

@9il
Copy link
Member

9il commented Aug 3, 2016

This PR looks like overengineering, imho

@burner
Copy link
Member Author

burner commented Aug 3, 2016

OK, so what about just keeping

bool testBit(T)(const T bitfield, const ulong idx) if(isIntegral!T);
T setBit(T)(const T bitfield, const ulong idx, bool value) if(isIntegral!T);
T flipBit(T)(const T bitfield, const ulong idx) if(isIntegral!T);

and adding an %B to format so it pretty prints like the bits as my prettyPrint function does?

@9il
Copy link
Member

9il commented Aug 3, 2016

Yes, this looks good to me

@9il
Copy link
Member

9il commented Aug 3, 2016

Why idx is not uint?

@burner
Copy link
Member Author

burner commented Aug 3, 2016

one less cast

@9il
Copy link
Member

9il commented Aug 3, 2016

Why ulong is default integer type for 1UL?

@burner
Copy link
Member Author

burner commented Aug 3, 2016

I do not understand the question?

@9il
Copy link
Member

9il commented Aug 3, 2016

I mean that this will be bad code for 32 bit DMD:

     ulong value = cast(ulong) bitfield;
     ulong mask = 1UL << idx;
     value |= mask;
     return cast(T) value;

@burner
Copy link
Member Author

burner commented Aug 3, 2016

If that is bad code (I assume by bad code you, will have a bug, not bad codegen), than the autotester is broken. Because I test that case and the autotester does not complain.

@9il
Copy link
Member

9il commented Aug 3, 2016

Sorry for my explanation. I mean bad code optimisation

@burner
Copy link
Member Author

burner commented Aug 3, 2016

what do you mean by bad code optimization?

@9il
Copy link
Member

9il commented Aug 3, 2016

what do you mean by bad code optimization?
Hide all checks

32 bit CPU has not 64 bit integer registers. So using ulong for all types is not optimal

@burner
Copy link
Member Author

burner commented Aug 3, 2016

I get it now. I'll work on making the cast to uint if possible

@wilzbach
Copy link
Contributor

@burner btw what about core.bitop?

  • bt - tests the bit
  • btc - test & complement
  • btr - test & resets (0)
  • bts - test & sets (1)

@burner
Copy link
Member Author

burner commented Aug 10, 2016

they work only on size_t*

enum upTo = T.sizeof * 8UL;
}

private template minUnsignedIntegral(T)
Copy link
Member

Choose a reason for hiding this comment

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

What's wrong with just using Unsigned!T?

@andralex
Copy link
Member

This kind of stuff should not be in Phobos.

@andralex andralex closed this Aug 15, 2016
@JackStouffer
Copy link
Contributor

JackStouffer commented Aug 15, 2016

@andralex For a module called bitmanip, a bit setting function seems like something that should be standard, no? The setBit and flipBit functions seem to be useful utilities to save the user the time they would spend using trial and error to get the correct result from int promotion.

I think that closing this was a bit premature.

@andralex
Copy link
Member

I'll reopen for further discussion, but one-liners such as setBit and flipBit don't hold much promise.

@andralex andralex reopened this Aug 15, 2016
@WalterBright
Copy link
Member

I'm sorry this has been a fair amount of work for you guys, in the light of what I'm about to say. But it falls into something I've argued against for years - a mass of trivial one-liners that take more time to read the documentation than just using & | and ^ operators. Avoiding learning about the trivial integral promotion rules is not a justification for this - and this is much, much more to learn than that - and the programmer will still have to learn about integral promotion. And if & | ^ are too hard for a user and must be put in a library, are + - * / next?

441 lines of source text to support 8 one liners and 2 trivial functions is an indication as well that something has gone awry.

This needs a much more compelling rationale, but I agree with Andrei that it's hard to see much promise in them. I apologize for not noticing this sooner, I wish I could have saved you the effort.

@wilzbach
Copy link
Contributor

wilzbach commented Aug 15, 2016

This needs a much more compelling rationale, but I agree with Andrei that it's hard to see much promise in them. I apologize for not noticing this sooner, I wish I could have saved you the effort.

FWIW I will try to give a rationale:

  1. While I agree that one-liners take more time to find than to write, is this really nice code to read?
cast(T)(cast(ulong)(bitfield) ^ (-cast(ulong)(value) ^ cast(ulong)(bitfield)) & (1UL << 8));

... instead of:

setBit!8(bitfield, 1)
  1. There's a performance difference if the mask can be created at CTFE
  2. Bit pretty-printing is something very helpful (but that will be a separate PR for std.format)
  3. It's already part of our library API - core.bitop provides convenience wrapper, but they have such ugly names than no one can find them (& they just work on size_t*)

Note: in previous discussions this PR has been boiled down to testBit, setBit and flipBit.

@9il
Copy link
Member

9il commented Aug 15, 2016

cast(T)(cast(ulong)(bitfield) ^ (-cast(ulong)(value) ^ cast(ulong)(bitfield)) & (1UL << 8));

This example has a lot of pointless casts.

I agree with @WalterBright

@andralex
Copy link
Member

The corollary here is that the set of people who simultaneously need to set bits in integrals and don't know how to is near empty.

@dnadlinger
Copy link
Contributor

dnadlinger commented Aug 15, 2016

As mentioned much earlier, I agree with Walter's and Andrei's position – I should have probably phrased my comment less politely. Higher-level wrappers like bitfields can add value by introducing an, as it were, logical abstraction on top of the physical bit-twiddling layer. For the latter, though, anything on top of the raw operations is not likely to be worth the increased baggage.

core.bitop is not a convenience wrapper – its API is rather horrid for high-level use, in fact –, but was intended to make sure that DMD emits the corresponding x86 instructions.

@andralex
Copy link
Member

I should have probably phrased my comment less politely.

Instead, I should have phrased my comment more politely. Thanks for being civil, my way of putting things is not called for.

@burner
Copy link
Member Author

burner commented Aug 15, 2016

The corollary here is that the set of people who simultaneously need to set bits in
integrals >and don't know how to is near empty.
[citation needed]

@WalterBright I disagree, algebra and integer promotion makes sense, bit fiddling and integer promotion does not. That is why I created these functions. I properly got carried away with some functions, but setting and testing individual bits via functions makes room in my harddrive to focus on more elaborate tasks.

@WalterBright
Copy link
Member

cast(T)(cast(ulong)(bitfield) ^ (-cast(ulong)(value) ^ cast(ulong)(bitfield)) & (1UL << 8));

Only one cast to ulong is needed, the rest will automatically happen. Library functions are not a solution to not understanding integral promotions rules - they are a fundamental, basic feature of the language, not some weird corner case.

core.bitop exists for two reasons:

  1. as mentioned before, ensure that the compiler generates the special instructions for them
  2. the bit test functions work on arrays that are more than one machine word (as do their corresponding special instructions)

@burner
Copy link
Member Author

burner commented Aug 16, 2016

Library functions are not a solution to not understanding integral promotions rules

Actually, they are!

For normal math you don't need to know integer promotion rules, they just work 99.999% of the time. And that is why they are good. For bit fiddling you need to know, or you use these functions.

@andralex
Copy link
Member

Sadly I don't see anything salvageable in here. It would be nice to migrate the bits printer to format as a flag for formatting integrals. I suggest closing this.

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.

8 participants