2
\$\begingroup\$

BitPat here is a wrapper around a bitset.

class BitPat
{
private:
    boost::dynamic_bitset<> data;
public:    
    bool BitPat::operator[](size_t coord) const
    {
        if(coord < length)
            return data[coord];
        else
            throw std::out_of_range(std::string("Pattern coord out of range")
                + " (" + std::to_string(coord%_width) + ", " + std::to_string(coord/_width) + ") "
                + "[" + std::to_string(coord) + "]");
    }

    // vs:

    bool BitPat::operator[](size_t coord) const
    {
        if(coord >= _length){
            throw std::out_of_range(std::string("Pattern coord out of range")
                + " (" + std::to_string(coord%_width) + ", " + std::to_string(coord/_width) + ") "
                + "[" + std::to_string(coord) + "]");
        }
        return data[coord];
    }
};

I have additional getters and setters where I'd implement bounds-checking like this. It generates a lot of boilerplate code though.
As a first step maybe I should define some little formatting functions, but most importantly I'm curious which version of the if statements is more conventional.
Not sure if omitting return statements like in the first case is good practice either.

\$\endgroup\$
2
\$\begingroup\$

It doesn't really matter which direction you do the checking in - it's a matter of personal preference. Both are valid, semantically identical, and will almost certainly perform identically.

One thing you can do to avoid having to rewrite all of this stuff is to have a single function that does bounds-checking:

size_t boundsChecked(size_t coord) const {
    // whichever order of this you prefer
    if (coord >= _length) {
        throw std::out_of_range(...);
    }
    return coord;
}

And then defer to that function everywhere you want to add bounds checking:

bool operator[](size_t coord) const
{    
    return data[boundsChecked(coord)];
}
\$\endgroup\$
  • \$\begingroup\$ I like that method, gives the needed extensibility \$\endgroup\$ – MatrixAndrew Nov 3 '15 at 21:50
  • \$\begingroup\$ Actually, while they are semantically identical they are unlikely to perform equally well: Remember that modern CPUs are pipelined, and a branch-misprediction is really costly. \$\endgroup\$ – Deduplicator Nov 4 '15 at 14:48
  • 1
    \$\begingroup\$ @Deduplicator Why would one be more likely to mispredict than the other? We're checking the same thing. \$\endgroup\$ – Barry Nov 4 '15 at 15:03
  • \$\begingroup\$ You could look for "static branch-prediction". But I don't know how the compiler decides to generate the code, so I cannot say whether the if-statement will do a forward-branch for its body or not. \$\endgroup\$ – Deduplicator Nov 4 '15 at 15:07
1
\$\begingroup\$

You know, your class is a golden opportunity to let private inheritance shine.

All those functions where you don't want to add any checking, just use a using-declaration to make them public.

The others, reimplement with delegation after additional checking, using a dedicate check_or_throw-function like Barry recommends.

Regarding the normal way to do things, that's first checking the preconditions, then doing the main work.
And where in your first snippet are you omitting any return-statement? I hope you are running with full warnings (-Wall -Wextra -pedantic), and the compiler should not complain.

gcc and clang have __builtin_expect, which you can use to give the compiler a hint.
Also see: https://stackoverflow.com/questions/1440570/likely-unlikely-equivalent-for-msvc

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.