Preferred style for C++ catch blocks

maintainers
#1

I have a question about the catch block for try/catch combinations. I am using the try statement without caring what the specific error is, just that an error was thrown. Hence, I used the catch (...) construct which will capture anything. To show what I mean, here is the code in its entirety:

      try
        {
          tmp_rgb[0] = static_cast<double> (stoi (str.substr (1,1), nullptr, 16))
                       / 15.0;
          tmp_rgb[1] = static_cast<double> (stoi (str.substr (2,1), nullptr, 16))
                       / 15.0;
          tmp_rgb[2] = static_cast<double> (stoi (str.substr (3,1), nullptr, 16))
                       / 15.0;
        }
      catch (...)
        {
          retval = false;
        }

The function stoi is part of the std library so it will throw a variable of type std::exception. Should I explicitly look for that? To my mind, it seems overly verbose to include that type information when I have no plans to use it. I guess I think of it similarly to using the auto keyword when I don’t particularly care to know what the type is.

#2

Agree to your position :+1: In your particular case ... clearly documents, if the try-block fails, there is nothing better that can be done. Giving an explicit type in the catch block makes me having to look up, what can all be thrown inside this try-block.

#3

We may need to be careful about using “catch (…)” as that will also catch interrupt exceptions and that’s probably not what want when you intend to just catch execution errors.

#4

Just to understand how serious this is, how often would an interrupt be likely to occur and get caught by (...) as opposed to going to the interrupt handler? It seems to me that the probability is something like probability of running this section of code AND probability an interrupt is called. Since, as far as I know, the two are independent events the formula should be P(A) AND P(B) = P(A)*P(B).

The probability of running this section of code is the time it takes to execute this code divided by the total runtime. These are simple library calls so I might figure something like 20 milliseconds per call or a total of 60 milliseconds for the entire block. I don’t know the length of a typical interactive Octave session but I might guess 15 minutes. That would make the first probability 1/15,000.

How many interrupts does Octave typically receive in a 15 minute session? Maybe 5 Ctrl+C to abort something? Seems high to me, but will see where it leads. I’m guessing interrupt routines are slow, maybe 100 milliseconds per invocation. This would make the second probability 1/1,800. Total probability of hitting this would be 1 in 27 million. That’s reasonably low. On the other hand, Bit Error Rates for disk drives are more like 1 in 10^14, so there is room for improvement. I would say that the the fallback path through (...) is not bad. The function returns false and a try/catch block above this one then issues an error and returns the user to the prompt. So, if the interrupt had been a Ctrl+C, rather than say a SIGKILL, the user would end up in the same place (the command prompt) anyways.

If I change the catch block to (const octave::execution_exception&) are we guaranteed to capture everything except interrupts? Or are there additional exception classes that might be missed?

#5

[edited for clarity]

When an interrupt signal arrives, the signal handler runs and sets a global variable. The octave_quit() function checks this variable and throws the interrupt exception if it sees that an interrupt signal has occurred. So that interrupts are somewhat responsive, octave_quit is called in many places throughout Octave. So I think it is highly likely that an interrupt could end up in a catch(…) block.

The interrupt exception is derived from std::exception. Other execution errors are derived from std::runtime_error. So I think the solution is to catch octave::execution_exception or std::runtime_error instead of catching everything.

#6

Easy enough change to make. See http://hg.savannah.gnu.org/hgweb/octave/rev/a88f6107aceb.

Doing a grep for catch (...) shows one more questionable instance in gzip.cc

          try
            {
              X::zip (path, dest_path);
            }
          catch (...)
            {
              // Error "handling" is not including filename on the output list.
              // Also remove created file which maybe was not even created
              // in the first place.  Note that it is possible for the file
              // to exist in the first place and for X::zip to not have
              // clobber it yet but we remove it anyway by design.
              sys::unlink (dest_path);
              return;
            }

I don’t know what kind of exception xlib will throw. Will this also be caught by std::runtime_error and should that replace ... ?

#7

Using grep and looking through the catch blocks in Octave core I notice some inconsistencies.

  1. Some instances of exceptions are labeled with octave:: namespace and others are not. For example, in ov-fcn-handle.cc
                catch (index_exception&)
                  {
                    err_invalid_fcn_handle (m_name);
                  }
  1. Some instances do not declare the exception const, although no change appears to be made to the exception. For example, the code above or this code from ov-java.cc
      catch (std::string msg)
        {
          error ("%s", msg.c_str ());
        }

Am I correct that these are working, but not best practice?

#8

Also, should we have a consistent naming strategy for exceptions? I often see e which might stand for exception. But I also see ee which might stand for execution exception. There is even qde which is used for quit_debug_exception. Would it make sense to standardize the naming as well so that e refers to std::exception, ee to execution_exception, etc., etc.?

#9

Using consistent names seems like a good thing to me. I’ve opted for short names since they are used locally in the catch block.

#10

The octave:: namespace tag should only be used when it is needed outside of a namespace octave { ... } block. I expect that we just have some tags that weren’t removed when code was moved inside the namespace.

If possible, we should catch exceptions by const reference. Catching by value isn’t necessarily wrong, but it doesn’t seem like the best thing to do.

#11

If there are instances where we really don’t know what could be thrown and we want to handle everything, then I think we could do something like this:

catch (const interrupt_exception&)
  {
    // cleanup if needed...
    throw;
  }
catch (...)
  {
    // respond to all other types of exception
    ...
  }
#12

I just checked in a changeset (http://hg.savannah.gnu.org/hgweb/octave/rev/8f67ad8b3103) that cleans up the use of exceptions in Octave according to the recommendations in this thread:

  1. Exception variables are named after the first letters of their class so “execution_exception” -> “ee”, “index_exception” -> “ie”, etc.
  2. Where possible, const is used in the catch block prototype.
  3. Use of the (...) catch block is preceded by a catch block for interrupt_exceptions.
1 Like