bitfields2

[marler8997]

Based on discussion with @adamdruppe (http://forum.dlang.org/thread/ibwazxrngcsrfolgbefs@forum.dlang.org)

Added a new version of bitfields called bitfields2. The reason for creating bitfields2 instead of enhancing bitfields is because the design requires it to be a mixin template instead of a normal template. This change is crucial to how bitfields2 works, but means that any application using it must omit the extra set of parenthesis that bitfields requires, i.e.

mixin(bitfields!(int, "a", 8));
mixin bitfields2!(int "a", 8);

The main advantage of bitfields2 is that it is able to reference the field types by alias, whereas the current implementation converts each field type alias to a string and then mixes in the type name. I've included a small program to demonstrate the differences in each technique and where the weakness lies in the current technique.

import std.stdio, std.typecons;

// Each template allows you to declare a variable
template declareUsingCurrentBitfieldsTechnique(T...)
{
    enum declareUsingCurrentBitfieldsTechnique = T[0].stringof ~ " " ~ T[1] ~ ";";
}
mixin template declareUsingNewBitfieldsTechnique(T...)
{
    mixin("T[0] " ~ T[1] ~ ";");
}

void main()
{
    // example on how to use both, declare varables a and b using each technique
    {
        mixin(declareUsingCurrentBitfieldsTechnique!(int, "a"));
        mixin declareUsingNewBitfieldsTechnique!(int, "b");
        
        // use the declared variables
        a = 3;
        b = 8;
        writefln("a = %s, b = %s", a, b);
    }
    
    // show problem with current technique
    {
        // The current technique fails to compile because wen you convert T[0] to a string,
        // it refers to the template name "Flag" not the template instantiation "Flag!"myflag"".
        // There are ways to try to get the string for the template instantiation but they are
        // brittle and a general solution is very difficult and may not be possible with the current language features.
        mixin(declareUsingCurrentBitfieldsTechnique!(Flag!"myflag", "x"));
        mixin declareUsingNewBitfieldsTechnique!(Flag!"myflag", "y");
    }
}

P.S. I've also added the bitfields2Code template so that an application can see/grok the generated bitfield code if needed, i.e.

pragma(msg, bitfields2Code!(int, "a", 8));

[UplinkCoder]

did you benchmark the compiletime of this, against bitfields ?

[marler8997]

The new one was compiling a bit slower because I was using std.format to generate code. I've since replaced it with string appending, like the original one uses and now performance is basically the same. Here's the benchmark.

import std.stdio, std.conv, std.bitmanip;

void main()
{
    mixin(function()
    {
        string code;
        foreach(i; 0..300)
        {
            version(Original)
            {
                code ~= 
                    `static struct Bitfield`~ i.to!string ~ `
                    {
                        mixin(bitfields!(
                            uint, "x",    2,
                            int,  "y",    3,
                            uint, "z",    2,
                            bool, "flag", 1));
                    }`;
            }
            version(New)
            {
                code ~= 
                    `static struct Bitfield`~ i.to!string ~ `
                    {
                        mixin bitfields2!(
                            uint, "x",    2,
                            int,  "y",    3,
                            uint, "z",    2,
                            bool, "flag", 1);
                    }`;
            }
        }
        return code;
    }());
}

[marler8997]

@UplinkCoder
Update, I found a place in the code where I could combine a string concatenation, and after doing that the new one seems to consistently build faster. Very odd that string concatenation at compile time can make such a difference.

EDIT: Nevermind, my edit actually caused some of the code not to be generated, fixed it and now performance is still very similar to the original.

[UplinkCoder]

yeah std.format is a compile-time eating beast.

[codecov]

Codecov Report

Merging #5490 into master will decrease coverage by 0.02%.
The diff coverage is 75.97%.

@@            Coverage Diff             @@
##           master    #5490      +/-   ##
==========================================
- Coverage   88.45%   88.42%   -0.03%     
==========================================
  Files         120      120              
  Lines       78371    78523     +152     
==========================================
+ Hits        69320    69436     +116     
- Misses       9051     9087      +36

Impacted Files	Coverage Δ
std/bitmanip.d	94.37% <75.97%> (-1.85%)	⬇️
std/algorithm/sorting.d	98.65% <0%> (-0.07%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f91f8e2...06523d4. Read the comment docs.

[ghost]

Stop creating AST nodes for something that's done automatically (i.e default initialization).

[CyberShadow]

#5497 (comment)

[ghost]

ditto

[CyberShadow]

#5497 (comment)

[dlang-bot]

Thanks for your pull request and interest in making D better, @marler8997! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

[adamdruppe]

On Thu, Jun 22, 2017 at 06:05:04PM -0700, Basile Burg wrote: Stop creating AST nodes for something that's done automatically (i.e default initialization).

why?

[marler8997]

Seems there is a problem with circleci.sh...is this a known error? If it's been fixed can someone retrigger the build?

./circleci.sh has_public_example
+ HOST_DMD_VER=2.068.2
+ DSCANNER_DMD_VER=2.071.2
++ curl --version
++ head -n 1
+ CURL_USER_AGENT='CirleCI curl 7.22.0 (x86_64-pc-linux-gnu) libcurl/7.22.0 OpenSSL/1.0.1 zlib/1.2.3.4 libidn/1.23 librtmp/2.3'
+ DUB=/home/ubuntu/dlang/dub/dub
+ N=2
+ CIRCLE_NODE_INDEX=0
+ case $CIRCLE_NODE_INDEX in
+ MODEL=64
+ case $1 in
+ echo 'Unknown command'
Unknown command
+ exit 1

./circleci.sh has_public_example returned exit code 1

[wilzbach]

Seems there is a problem with circleci.sh...is this a known error?

Yes.

If it's been fixed can someone retrigger the build?

You need to rebase to master..

[andralex]

@marler8997 can you please show a simple example "this would not work (or would be awful) with bitfields, but would work very nicely with bitfields2"? Thanks!

[marler8997]

struct Foo
{
    mixin(bitfields!(
        Flag!"foo", "foo", 1, // Error: std.typecons.Flag(string name) is used as a type
        void, null, 7));
}

struct Foo
{
    mixin bitfields2!(
        Flag!"foo", "foo", 1,
        void, null, 7);
}

[marler8997]

I should also mention that I created another PR to fix the specific use case I just gave as an example, but it is brittle and can't work in general, hence, why this PR was created which does work in the general case.

[wilzbach]

I am not sure that exposing a debug function is a good idea. As a user I would expect a standard library function to work ...

[marler8997]

The purpose of having it isn't to allow a developer to "debug" the implementation but to see/understand the code being generated. Maybe that's not a good enough reason though?

[marler8997]

I see in my description that I explicitly said the purpose of this function was to "debug" the code :) Woops!

[wilzbach]

I don't want to start a bikeshedding discussion, but bitfield2 is a rather non-descriptive name (unfortunately I don't have a better suggestion).
Also if we really want to do this, we should at least deprecate bitfield, s.t. we don't end up with both for eternity.

[marler8997]

Cleaning up old PRs that aren't going anywhere