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

Fix Issue 21628 - The padding bits of bitfields could be calculated automatically #7787

Closed
wants to merge 1 commit into from

Conversation

berni44
Copy link
Contributor

@berni44 berni44 commented Feb 11, 2021

I always wondered, why the mixin could not create the padding bits automatically. The solution was much simpler, than I thought... :-)

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @berni44! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
21628 enhancement The padding bits of bitfields could be calculated automatically

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7787"

@RazvanN7
Copy link
Collaborator

RazvanN7 commented Feb 11, 2021

Maybe the limitation was put in place to make sure that the user realizes that the bitfield size is a power of 2? Imagine that you have two bitfields in one struct, each of length 48. With this patch it is not obvious if the final size of the struct is going to be 96 or 128. With the limitation in place, the user is forced to realize that the size is going to be 128. cc @andralex

std/bitmanip.d Outdated
The sum of all bit lengths in one $(D_PARAM bitfield) instantiation
must be exactly 8, 16, 32, or 64. If padding is needed, just allocate
one bitfield with an empty name.
In former versions of Phobos the sum of all bit lengths in one
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no point in highlighting what older versions did in the documentation.

Suggested change
In former versions of Phobos the sum of all bit lengths in one
The sum of all bit lengths in one $(D_PARAM_bitfield) instantiation must be smaller than 64.

@berni44
Copy link
Contributor Author

berni44 commented Feb 11, 2021

Maybe the limitation was put in place to make sure that the user realizes that the bitfield size is a power of 2?

Maybe this should be explained in the docs?

@andralex
Copy link
Member

This was by design. The layout is important and it is good to be explicit. This is in contrast with C's cavalier approach that is loosely defined and left to platforms, which makes people avoid it in portable code.

@andralex
Copy link
Member

BTW, one good thing to do with bitfields would be refactoring the implementation, which is quite convoluted and uses ancient technology. E.g. even the definition (template with an alias inside) could be replaced with a templated alias.

@berni44
Copy link
Contributor Author

berni44 commented Feb 11, 2021

I added a detailed documentation. @andralex Do you think this makes things explicit enough?

@berni44
Copy link
Contributor Author

berni44 commented Feb 11, 2021

BTW, one good thing to do with bitfields would be refactoring the implementation, which is quite convoluted and uses ancient technology. E.g. even the definition (template with an alias inside) could be replaced with a templated alias.

I'd like to leave this for an other PR. I also thought of adding more error checking (number of args is dividable by 3 etc.) and some more unittests and public examples.

@berni44
Copy link
Contributor Author

berni44 commented Feb 11, 2021

Changed the second unittest, to show how a larger storage type can be enforced.

@andralex
Copy link
Member

I added a detailed documentation. @andralex Do you think this makes things explicit enough?

Of course more docs are better than fewer, but I disagree with the feature in the first place. But reasonable people may disagree.

Changed the second unittest, to show how a larger storage type can be enforced.

This is evidence that the proposed feature adds more trouble than it removes.

@berni44
Copy link
Contributor Author

berni44 commented Feb 12, 2021

OK. Got it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants