Can this bitset generation be made better or more efficient?

Question

Problem: Given an integer led as input, create a bitset (16 bits) with led bits set to 1. Then, create the following sequence (assume in this case led = 7):

0000000001111111
0000000000111111
0000000001011111
0000000001101111
0000000001110111
0000000001111011
0000000001111101
0000000001111110

Note that it is a "zero" that is moving to the right. The code I wrote is:

void create_mask(int led){
    string bitString;
    for (int i = 0; i < led; i++){
        bitString += "1";
    }
    bitset<16> bitMap(bitString);

    for (int i = led; i >= 0; i--){
        bitMap[i] = false;
        cout << bitMap << endl;
        bitString = "";
        for (int j = 0; j < led; j++){
            bitString += "1";
        }
        bitMap = bitset<16> (bitString);
    }
}

I don't like the nested loop where I set each bit to 0. I think that could be made better with less complexity.

The limiting factor here is likely the time it takes for the LED to light up, for the human eye to recognize it, and for the LED to turn off. Trying to optimize code that's already 10,000 times faster than the physical device it's driving is a fool's errand. — JohnFilleau, Aug 04 '20 at 00:49
But look at the documentation for `std::string` constructors [here](https://en.cppreference.com/w/cpp/string/basic_string/basic_string). There's a constructor that makes the string full of 1's immediately instead of appending a bunch of 1's. — JohnFilleau, Aug 04 '20 at 00:51
@JohnFilleau Oh, it is not related to a physical led at all. It is a function made to solve one of the UVA problems: https://onlinejudge.org/index.php?option=com_onlinejudge&Itemid=8&page=show_problem&problem=2146 — Miguel Duran Diaz, Aug 04 '20 at 00:51
And instead of creating a bitset from a string, you can create a bitset from an integer. — JohnFilleau, Aug 04 '20 at 00:53
Take a look at its [constructors](https://en.cppreference.com/w/cpp/utility/bitset/bitset). You may prefill `0000000001111111` to your bitset then all you need is flipping specific bit to zero. — Louis Go, Aug 04 '20 at 01:00
@JohnFilleau I didn't ask people to do my work. I did mine as you can see in the question. The code I posted actually works, only I didn't come up with the most efficient implementation. If I take your argument. the code review community would be a bunch of people asking others to do their work, which make no sense. Similar sites (codewars, for example) work like that too. You try your solution and after you correctly solved an exercise, you can see other's implementation to see how you could have done better. — Miguel Duran Diaz, Aug 04 '20 at 01:15
@JohnFilleau Also, I don't see any reason why my question should be downvoted, as it is a clear question and it does show research effort from my part. — Miguel Duran Diaz, Aug 04 '20 at 01:16
Without knowledge of how you’ll use it, it’s hard to say what sort of optimization would be appropriate. So this question is pretty much unanswerable. At the very least you need to say what microcontroller it is for (be *specific*), and where does the `bitset` come from? For a class assignment, the rule I have is: if it works and passes the tests, that’s more or less it. If this is for an actual hardware: you have plenty of explaining to do. — Kuba hasn't forgotten Monica, Aug 04 '20 at 01:18
I admit my initial downvote was because it looked like you were just looking for a higher score on your online judge. I have a bias against online judges, and I have a bias against people submitting others' work as their own. But you don't appear to have done any research on how to improve this. I think this would be a better question for [codereview.se] if you just want improvements. — JohnFilleau, Aug 04 '20 at 01:22

person · Accepted Answer · 2020-08-04T01:19:28.790

This is what I came up with:

void createMask(int len) {
    std::bitset<16> bitMap;
    for (int i = 1; i < len; i++)
    {
        bitMap.set();
        bitMap >>= 16 - len;
        bitMap[len - i] = false;
        std::cout << bitMap << std::endl;
    }
}

bitMap.set() sets all bits in the bitset to 1 (or true) bitMap >>= 16 - len shifts all the bits to the right but it does it 16 - 7 (if len was 7) so there are 9 zeros and seven ones. bitMap[len - i] = false sets the bit at 7 - i to 0 (or false). len - i is a way of specifying the inverse number (basically it starts setting the zeros on the left and works towards the right depending on the value of i) The loop starts at 1 because you're setting the bit to 0 anyways and prevents program from crashing when len is 16 –

score 0 · Answer 2 · answered Aug 04 '20 at 01:01

If you want to use a std::bitset, you can take advantage of the bit functions like shifting and XOR'ing. In this solution I have a base bitset of all ones, a mask that shifts right, and I output the XOR of the two on each iteration.

Untested.

void output_masks(int bits, std::ostream& os){
    std::bitset<16> all_ones((1 << bits) - 1);
    std::bitset<16> bit_mask(1 << (bits - 1));

    while (bit_mask.any()) {
        os << (all_ones ^ bit_mask);
        bit_mask >>= 1;
    }
}

Can this bitset generation be made better or more efficient?

2 Answers2