Copy arbitrary number of bits at arbitrary offset from buffer to another buffer

Question

I have just written a function copy_lowbits_off to copy any number of bits (not bytes) from a source buffer to a destination buffer. The function also supports arbitrary offset (expressed in bits) in the source and the destination buffer.

Could you have a look at it and let me know if anything can be improved in style, design or best practices? Specifically, I find my code too complicated for what it does and I am rather unhappy by the naming of functions and variables.

I compile in C++11 but I don't use much of the C++ features here.

#define BITMASKU8(x) ((1U << (x)) - 1)
/**
 * Select low_sel bits from src starting from the lower bit and
 * copy the selected bits to dst at offset msb_off, starting from the most
 * significant bit.
 * The caller must ensure that low_sel + msb_off <= 8
 *
 * @param dst: Destination byte
 * @param src: Source byte
 * @param low_sel: Number of low bits to select from src
 * @param msb_off: Offset from msb in dst
 */
void offset_bitcpy(std::uint8_t& dst, const std::uint8_t src,
        std::uint8_t low_sel, std::uint8_t msb_off) {
    const std::uint8_t sel_src = src & BITMASKU8(low_sel);
    std::uint8_t shift = 8 - (msb_off + low_sel);
    const std::uint8_t shift_src = sel_src << shift;
    dst |= shift_src;
}

/**
 * Copy an arbitrary number of *bits* from an arbitrary position (expressed
 * in bits) in the src buffer, to an abitrary position (expressed in *bits*) in
 * the dst buffer.
 * The caller must ensure that dst_offbits < 8 && src_offbits < 8. If you need
 * to have a bit offset > 8, set dst += bit offset / 8 and
 * dst_offbits = bit offset % 8 (respectively for src). low_sel may be >= 8.
 *
 * @param dst: Destination buffer
 * @param src: Source buffer
 * @param low_sel: Number of bits to copy from src to dst
 * @params src_offbits: [0,7] Offset in the first byte of the source buffer
 * @params dst_offbits: [0,7] Offset in the second byte of the destination
 *      buffer
 */
void copy_lowbits_off(std::uint8_t* dst, const std::uint8_t* src,
        unsigned low_sel, std::uint8_t dst_offbits, std::uint8_t src_offbits) {
    while(low_sel != 0) {
        // Number of bits to select from the first byte of src, and write
        // to the first byte of dst
        const std::uint8_t sel_byte  = std::min(low_sel, 8U - dst_offbits);
        const std::uint8_t off_src = *src >> src_offbits;
        offset_bitcpy(*dst, off_src, sel_byte, dst_offbits);
        // Update dst offsets
        const std::uint8_t add_dst_off = sel_byte + dst_offbits;
        dst += add_dst_off / 8;
        dst_offbits = add_dst_off % 8;
        // Update src offsets
        src_offbits += sel_byte;
        src += src_offbits / 8;
        src_offbits = src_offbits % 8;
        low_sel -= sel_byte;
    }
}

Answer 1

I don't know how you could make it shorter or less complicated, hopefully someone else will provide you a few insights on that. I can comment on a few minor points that you might be interested in changing:

• BITMASKU8 should preferably be a function. Such a short function would certainly to be inlined by the compiler, producing the same results as the macro you currently have. If you can't afford the chance of overhead, or your compiler is not capable of inlining, then you should at least #undef the macro name at the end of the file to clear the namespace.

• In offset_bitcpy(), I see that you've nicely marked the variables that are initialized only once with const, this is a good practice, even for small functions/blocks, you never know how big they might grow in the future, after a few refactorings. But you did forget to mark std::uint8_t shift as const too, since it is never modified after being set.

• Decide how you are going to handle invalid inputs. The only thing that can break your code is an ill-formed input in copy_lowbits_off(), such as a null pointer or a zero count. Decide which course of action should be taken in such cases. Just doing nothing is hardly a good choice. At least assert to facilitate debugging, or throw and exception if the caller should be responsible for handling the error.

• Overall, I find your code too tightly packed. Blank lines before each comment inside copy_lowbits_off() would visually separate stuff in a more natural way, IMHO.