2

I am writing a small wrapper around OpenGL so that I can use it in an object-oriented way. I have created a class called a buffer, and in its constructor I would like it to validate the type of buffer that it has been passed against lots of GLenum types. Here is the relevant portion of the code:

Buffer::Buffer(GLenum type) {

    // Validate 'type'.
    switch (type) {
        // *** These are the valid values ***
        case GL_ARRAY_BUFFER:
        case GL_ATOMIC_COUNTER_BUFFER:
        case GL_COPY_READ_BUFFER:
        case GL_COPY_WRITE_BUFFER:
        // *** Lots more valid values here... ***
            break; // Jump out of the switch statement.

        default:
            throw std::logic_error("Invalid GLenum 'type' in Buffer constructor.");
    }

    type_ = type;
    target_ = target;
    glGenBuffers(1, &buffer_);

}

In this code, the switch statement would step in to the appropriate value, and pass through the other valid values until it broke out of the switch statement. All other values would step into the default clause and throw an exception. I could, however, also use an if statement:

if (type != GL_ARRAY_BUFFER &&
    type != GL_ATOMIC_COUNTER_BUFFER &&
    type != GL_COPY_READ_BUFFER &&
    type != GL_COPY_WRITE_BUFFER &&
    // *** Lots more valid values here... ***
    ) {
        throw std::logic_error("Invalid GLenum 'type' in Buffer constructor.");
    }

    // ...
}

However, this uses a lot of repetition of type != and isn't necessarily any clearer. Is there an accepted convention when doing this, for code clarity and for speed? Should I be validating my inputs at all, when not doing so could cause undefined behaviour?

Alex V-P
  • 49
  • 7
  • First question: can whatever function produced the `type` value actually return an invalid value? –  Feb 17 '17 at 16:50
  • @NeilButterworth The `type` value is generally hard-coded in, and doesn't rely on a function -- but my classmates are going to be using this `class` and I can't rely on them to follow my documentation. – Alex V-P Feb 17 '17 at 16:54
  • 1
    http://stackoverflow.com/questions/97987/advantage-of-switch-over-if-else-statement A switch is much more likely to generate efficient code, even with a 20 year old compiler. – Olivier Feb 17 '17 at 16:54
  • @Olivier I guessed that the `switch` would be more efficient, but is there any difference from a code clarity perspective (especially with the intentional fallthroughs?) – Alex V-P Feb 17 '17 at 16:57
  • Why not use `assert` instead of an exception? Using an invalid type is a bug and means that your code does not work the way you thought it did. What exactly would you intend to *do* in a `catch (std::logic_error const& exc)`? – Christian Hackl Feb 17 '17 at 17:07
  • @ChristianHackl It was to provide a more convenient debug message than `assert(false);`. I think any exceptions are just being printed to standard output. – Alex V-P Feb 18 '17 at 02:25
  • @AlexV-P: An assertion, if not disabled with `NDEBUG`, terminates the application immediately, which is typically the best thing that can happen if a bug is detected at runtime. An exception is part of the designed program flow and should not be confused with bug detection. – Christian Hackl Feb 18 '17 at 11:04
  • @Alex I find the switch clearer as well as there is less syntax to parse to understand it. That's the kind of thing switch is meant for. – Olivier Feb 18 '17 at 15:00

1 Answers1

4

I would use a std::set if there are many values:

static std::set<int> invalidValues = 
     { GL_ARRAY_BUFFER 
     , GL_ATOMIC_COUNTER_BUFFER
     , GL_COPY_READ_BUFFER
     , GL_COPY_WRITE_BUFFER        
     // ...
     };

if(invalidValues.find(type) != invalidValues.end()) {
    throw std::logic_error("Invalid GLenum 'type' in Buffer constructor.");
}

This will serve for speed and better readability.

Should I be validating my inputs at all, when not doing so could cause undefined behaviour?

You never want undefined behavior in your program. You can't rely this would cause an exception (crash) or such.

πάντα ῥεῖ
  • 1
  • 13
  • 116
  • 190
  • This is exactly what I was looking for. Thanks! – Alex V-P Feb 17 '17 at 16:57
  • 1
    If the values in the set don't need to be sorted consider using `std::unordered_set` and if you don't actually need to use the result of `std::find` consider using `std::any_of` or `std::none_of` – YoungJohn Feb 17 '17 at 17:08
  • 1
    @YoungJohn In this case, it looks like an `std::unordered_set` and `std::none_of` are more appropriate. – Alex V-P Feb 18 '17 at 02:42