Topics

Imath: Options for Exceptions

Owen T.
 

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code,
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17

Larry Gritz
 

Oh, these code examples are extremely helpful.

I think Option 1 is inadequate, since it would preclude having the exception-free versions declared properly as 'noexcept'. I think that's an important aspect of this modernization (as well as adding 'noexcept' to the many functions that never had exceptions in the first place).

Aesthetically, I prefer option 2, though it has two acknowledged limitations: (a) On a case-by-case basis, we would need to agree on what would be returned in the error cases (are error-indicating values possible? what should they be?). (b) What to do if/when there are cases where more than one kind of error could occur, and if it's important for calling code to distinguish among them.

Option 3 handles those issues, but despite the precedent from std::filesystem, I think it's kind of ugly how it requires calling code to be littered with declarations of err_code variables, even if you have no intention of checking them (for example, if you know that the surrounding code makes the error condition impossible).

I can think of two other possible approaches (I'm merely listing them for consideration, not necessarily preferring them at this time):

4. Further template on whether we want exceptions (do I remember Kimball Thurston advocating this?).

5. To the extent that we end up with a header-only library, we could allow a preprocessor symbol (set by user code before #include of the Imath headers) to make a strong guarantee of exceptions or not:

#ifndef IMATH_EXCEPTIONS
#  define IMATH_EXCEPTIONS /* bikeshed color goes here */
#endif

#if IMATH_EXCEPTIONS
template<class T>
T Frustum<T>::aspect() const
{
    if (error case)
        throw ...
    return rightMinusLeft / topMinusBottom;
}
#else
template<class T>
T Frustum<T>::aspect() const noexcept
{
    if (error case)
        return T(0);
    return rightMinusLeft / topMinusBottom;
}
#endif


Just out of curiosity, have you done enough of an inventory to know how many functions total we are dealing with that throw exceptions now?


On Jun 11, 2020, at 11:53 AM, Owen T. via lists.aswf.io <ownthmpsn=protonmail.ch@...> wrote:

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code,
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17


--
Larry Gritz




Matt Pharr
 

Drive-by: I've seen a nice idiom for this sort of thing in an exception-free codebase that also still gives you error codes (which options 1, 2, and 5 lose). It's effectively

template <typename T, typename ErrorCode>
class ResultOr {
  ErrorCode error;
  std::optional<T> value;
};

with a bunch of methods around it. Methods return a ResultOr object, so can return either a result or an error code, just via "return" statements, thanks to various ResultOr constructors. In the caller, it's then a runtime error to try to access "value" if it wasn't set (so this enforces checking the error code and dealing with it.)

Matt


On Thu, Jun 11, 2020 at 12:29 PM Larry Gritz <lg@...> wrote:
Oh, these code examples are extremely helpful.

I think Option 1 is inadequate, since it would preclude having the exception-free versions declared properly as 'noexcept'. I think that's an important aspect of this modernization (as well as adding 'noexcept' to the many functions that never had exceptions in the first place).

Aesthetically, I prefer option 2, though it has two acknowledged limitations: (a) On a case-by-case basis, we would need to agree on what would be returned in the error cases (are error-indicating values possible? what should they be?). (b) What to do if/when there are cases where more than one kind of error could occur, and if it's important for calling code to distinguish among them.

Option 3 handles those issues, but despite the precedent from std::filesystem, I think it's kind of ugly how it requires calling code to be littered with declarations of err_code variables, even if you have no intention of checking them (for example, if you know that the surrounding code makes the error condition impossible).

I can think of two other possible approaches (I'm merely listing them for consideration, not necessarily preferring them at this time):

4. Further template on whether we want exceptions (do I remember Kimball Thurston advocating this?).

5. To the extent that we end up with a header-only library, we could allow a preprocessor symbol (set by user code before #include of the Imath headers) to make a strong guarantee of exceptions or not:

#ifndef IMATH_EXCEPTIONS
#  define IMATH_EXCEPTIONS /* bikeshed color goes here */
#endif

#if IMATH_EXCEPTIONS
template<class T>
T Frustum<T>::aspect() const
{
    if (error case)
        throw ...
    return rightMinusLeft / topMinusBottom;
}
#else
template<class T>
T Frustum<T>::aspect() const noexcept
{
    if (error case)
        return T(0);
    return rightMinusLeft / topMinusBottom;
}
#endif


Just out of curiosity, have you done enough of an inventory to know how many functions total we are dealing with that throw exceptions now?


On Jun 11, 2020, at 11:53 AM, Owen T. via lists.aswf.io <ownthmpsn=protonmail.ch@...> wrote:

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code,
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17


--
Larry Gritz




Thiago Ize
 

std::optional, like most fancy c++ features, is neat on paper, and I'll probably find uses for it once I'm allowed to use C++17, but I get worried it'll still bring too much baggage. For instance, if you use the optional's value(), it'll cause the compiler to generate exception handling code (looking at the assembly I see a std::bad_optional_access can be thrown). If you use value_or() you can avoid the exceptions, which is nice, but you then need to supply the value to use when there's an error which can be either neat or annoying depending on the situation. Even when there's no error, both versions will never be as fast as code that does no error checking (not sure if that matters for imath).

For those curious in trying it out, here's my quick hack at implementing Matt's suggestion: https://gcc.godbolt.org/z/ZYFaap

Also, the vfx reference platform for 2020 looks to be c++14 (no mention of what will be in 2021), so that might be another impediment even if this is the cleanest approach.

Thiago


On Thu, Jun 11, 2020 at 1:43 PM Matt Pharr <matt.pharr@...> wrote:
Drive-by: I've seen a nice idiom for this sort of thing in an exception-free codebase that also still gives you error codes (which options 1, 2, and 5 lose). It's effectively

template <typename T, typename ErrorCode>
class ResultOr {
  ErrorCode error;
  std::optional<T> value;
};

with a bunch of methods around it. Methods return a ResultOr object, so can return either a result or an error code, just via "return" statements, thanks to various ResultOr constructors. In the caller, it's then a runtime error to try to access "value" if it wasn't set (so this enforces checking the error code and dealing with it.)

Matt


On Thu, Jun 11, 2020 at 12:29 PM Larry Gritz <lg@...> wrote:
Oh, these code examples are extremely helpful.

I think Option 1 is inadequate, since it would preclude having the exception-free versions declared properly as 'noexcept'. I think that's an important aspect of this modernization (as well as adding 'noexcept' to the many functions that never had exceptions in the first place).

Aesthetically, I prefer option 2, though it has two acknowledged limitations: (a) On a case-by-case basis, we would need to agree on what would be returned in the error cases (are error-indicating values possible? what should they be?). (b) What to do if/when there are cases where more than one kind of error could occur, and if it's important for calling code to distinguish among them.

Option 3 handles those issues, but despite the precedent from std::filesystem, I think it's kind of ugly how it requires calling code to be littered with declarations of err_code variables, even if you have no intention of checking them (for example, if you know that the surrounding code makes the error condition impossible).

I can think of two other possible approaches (I'm merely listing them for consideration, not necessarily preferring them at this time):

4. Further template on whether we want exceptions (do I remember Kimball Thurston advocating this?).

5. To the extent that we end up with a header-only library, we could allow a preprocessor symbol (set by user code before #include of the Imath headers) to make a strong guarantee of exceptions or not:

#ifndef IMATH_EXCEPTIONS
#  define IMATH_EXCEPTIONS /* bikeshed color goes here */
#endif

#if IMATH_EXCEPTIONS
template<class T>
T Frustum<T>::aspect() const
{
    if (error case)
        throw ...
    return rightMinusLeft / topMinusBottom;
}
#else
template<class T>
T Frustum<T>::aspect() const noexcept
{
    if (error case)
        return T(0);
    return rightMinusLeft / topMinusBottom;
}
#endif


Just out of curiosity, have you done enough of an inventory to know how many functions total we are dealing with that throw exceptions now?


On Jun 11, 2020, at 11:53 AM, Owen T. via lists.aswf.io <ownthmpsn=protonmail.ch@...> wrote:

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code,
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17


--
Larry Gritz




Owen T.
 

Concerning the inventory of exception containing functions in Imath:

Looking at this commit, it can be seen that all the exceptions were identified and changed as to not depend on Iex. This is around 50 calls to Iex exceptions contained in 24 functions with 5 of these already handled using OPTION 1.

Only 7 functions across two files are not in header files already. These would at the very least need to be moved into headers for OPTION 5. 

On 6/11/20 3:29 PM, Larry Gritz wrote:
Oh, these code examples are extremely helpful.

I think Option 1 is inadequate, since it would preclude having the exception-free versions declared properly as 'noexcept'. I think that's an important aspect of this modernization (as well as adding 'noexcept' to the many functions that never had exceptions in the first place).

Aesthetically, I prefer option 2, though it has two acknowledged limitations: (a) On a case-by-case basis, we would need to agree on what would be returned in the error cases (are error-indicating values possible? what should they be?). (b) What to do if/when there are cases where more than one kind of error could occur, and if it's important for calling code to distinguish among them.

Option 3 handles those issues, but despite the precedent from std::filesystem, I think it's kind of ugly how it requires calling code to be littered with declarations of err_code variables, even if you have no intention of checking them (for example, if you know that the surrounding code makes the error condition impossible).

I can think of two other possible approaches (I'm merely listing them for consideration, not necessarily preferring them at this time):

4. Further template on whether we want exceptions (do I remember Kimball Thurston advocating this?).

5. To the extent that we end up with a header-only library, we could allow a preprocessor symbol (set by user code before #include of the Imath headers) to make a strong guarantee of exceptions or not:

#ifndef IMATH_EXCEPTIONS
#  define IMATH_EXCEPTIONS /* bikeshed color goes here */
#endif

#if IMATH_EXCEPTIONS
template<class T>
T Frustum<T>::aspect() const
{
    if (error case)
        throw ...
    return rightMinusLeft / topMinusBottom;
}
#else
template<class T>
T Frustum<T>::aspect() const noexcept
{
    if (error case)
        return T(0);
    return rightMinusLeft / topMinusBottom;
}
#endif


Just out of curiosity, have you done enough of an inventory to know how many functions total we are dealing with that throw exceptions now?


On Jun 11, 2020, at 11:53 AM, Owen T. via lists.aswf.io <ownthmpsn=protonmail.ch@...> wrote:

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code,
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17


--
Larry Gritz




Matt Pharr
 

std::optional was admittedly a bad suggestion for a math library. Here’s an alternative where ResultOr just protects access to the value and asserts if you try to access it without checking the error code: https://gcc.godbolt.org/z/PEYKmx

If you do an apples-to-apples comparison where the error code is actually checked, then check() and check2() generate the same code. Amusingly, the assert() call in ResultOr::get() is optimized away because the calling code has checked the error code already.

Matt

On Jun 11, 2020, at 1:58 PM, Thiago Ize <thiago.ize@...> wrote:

std::optional, like most fancy c++ features, is neat on paper, and I'll probably find uses for it once I'm allowed to use C++17, but I get worried it'll still bring too much baggage. For instance, if you use the optional's value(), it'll cause the compiler to generate exception handling code (looking at the assembly I see a std::bad_optional_access can be thrown). If you use value_or() you can avoid the exceptions, which is nice, but you then need to supply the value to use when there's an error which can be either neat or annoying depending on the situation. Even when there's no error, both versions will never be as fast as code that does no error checking (not sure if that matters for imath).

For those curious in trying it out, here's my quick hack at implementing Matt's suggestion: https://gcc.godbolt.org/z/ZYFaap 

Also, the vfx reference platform for 2020 looks to be c++14 (no mention of what will be in 2021), so that might be another impediment even if this is the cleanest approach.

Thiago

On Thu, Jun 11, 2020 at 1:43 PM Matt Pharr <matt.pharr@...> wrote:
Drive-by: I've seen a nice idiom for this sort of thing in an exception-free codebase that also still gives you error codes (which options 1, 2, and 5 lose). It's effectively

template <typename T, typename ErrorCode>
class ResultOr {
  ErrorCode error;
  std::optional<T> value;
};

with a bunch of methods around it. Methods return a ResultOr object, so can return either a result or an error code, just via "return" statements, thanks to various ResultOr constructors. In the caller, it's then a runtime error to try to access "value" if it wasn't set (so this enforces checking the error code and dealing with it.)

Matt


On Thu, Jun 11, 2020 at 12:29 PM Larry Gritz <lg@...> wrote:
Oh, these code examples are extremely helpful.

I think Option 1 is inadequate, since it would preclude having the exception-free versions declared properly as 'noexcept'. I think that's an important aspect of this modernization (as well as adding 'noexcept' to the many functions that never had exceptions in the first place).

Aesthetically, I prefer option 2, though it has two acknowledged limitations: (a) On a case-by-case basis, we would need to agree on what would be returned in the error cases (are error-indicating values possible? what should they be?). (b) What to do if/when there are cases where more than one kind of error could occur, and if it's important for calling code to distinguish among them.

Option 3 handles those issues, but despite the precedent from std::filesystem, I think it's kind of ugly how it requires calling code to be littered with declarations of err_code variables, even if you have no intention of checking them (for example, if you know that the surrounding code makes the error condition impossible).

I can think of two other possible approaches (I'm merely listing them for consideration, not necessarily preferring them at this time):

4. Further template on whether we want exceptions (do I remember Kimball Thurston advocating this?).

5. To the extent that we end up with a header-only library, we could allow a preprocessor symbol (set by user code before #include of the Imath headers) to make a strong guarantee of exceptions or not:

#ifndef IMATH_EXCEPTIONS
#  define IMATH_EXCEPTIONS /* bikeshed color goes here */
#endif

#if IMATH_EXCEPTIONS
template<class T>
T Frustum<T>::aspect() const
{
    if (error case)
        throw ...
    return rightMinusLeft / topMinusBottom;
}
#else
template<class T>
T Frustum<T>::aspect() const noexcept
{
    if (error case)
        return T(0);
    return rightMinusLeft / topMinusBottom;
}
#endif


Just out of curiosity, have you done enough of an inventory to know how many functions total we are dealing with that throw exceptions now?


On Jun 11, 2020, at 11:53 AM, Owen T. via lists.aswf.io <ownthmpsn=protonmail.ch@...> wrote:

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code, 
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17


--
Larry Gritz







Peter Hillman
 


Ideally, existing code out there should compile without major modification and behave similarly to how it does now, which probably means IMath should be throwing exceptions by default and return types shouldn't change. Ideally, the solution should support libraries that want to compile against Imath, where those libraries want to offer the choice of throwing and no-throwing versions. Differently named functions would make that tedious, as would a compile-time preprocessor flag in Imath. It may also be that folks want exception handling on for a debug build only, and turn them off for an optimized release.

Perhaps the no-except versions of these functions should do very little error testing. The main reason for no-except versions is to avoid overhead, so maybe the test itself should be avoided. The no-except version of aspect() or normalize() might return infinity or NaN, and it's up to the caller to check for that, or sanitize the values up front for speed.
After all, it's as easy to check the result of aspect() for infinity/NaN as it would be to check an error object returned from aspect() and just as easy to forget to to do it too. Error checking in invert functions would still be required.

A variation on option #1 is to have overloaded functions that take a dummy parameter to indicate what the behavior should be, rather like type traits in STL.
You can pass an extra parameter to the function to make an explicit choice whether or not you want exceptions, but the type of that parameter controls the behavior, rather than its value. A single line change in user code can then turn exceptions on and off. Something along these lines: https://gist.github.com/peterhillman/b233c344918be066ad6704e378a71024


On 12/06/20 9:50 am, Matt Pharr wrote:
std::optional was admittedly a bad suggestion for a math library. Here’s an alternative where ResultOr just protects access to the value and asserts if you try to access it without checking the error code: https://gcc.godbolt.org/z/PEYKmx

If you do an apples-to-apples comparison where the error code is actually checked, then check() and check2() generate the same code. Amusingly, the assert() call in ResultOr::get() is optimized away because the calling code has checked the error code already.

Matt

On Jun 11, 2020, at 1:58 PM, Thiago Ize <thiago.ize@...> wrote:

std::optional, like most fancy c++ features, is neat on paper, and I'll probably find uses for it once I'm allowed to use C++17, but I get worried it'll still bring too much baggage. For instance, if you use the optional's value(), it'll cause the compiler to generate exception handling code (looking at the assembly I see a std::bad_optional_access can be thrown). If you use value_or() you can avoid the exceptions, which is nice, but you then need to supply the value to use when there's an error which can be either neat or annoying depending on the situation. Even when there's no error, both versions will never be as fast as code that does no error checking (not sure if that matters for imath).

For those curious in trying it out, here's my quick hack at implementing Matt's suggestion: https://gcc.godbolt.org/z/ZYFaap 

Also, the vfx reference platform for 2020 looks to be c++14 (no mention of what will be in 2021), so that might be another impediment even if this is the cleanest approach.

Thiago

On Thu, Jun 11, 2020 at 1:43 PM Matt Pharr <matt.pharr@...> wrote:
Drive-by: I've seen a nice idiom for this sort of thing in an exception-free codebase that also still gives you error codes (which options 1, 2, and 5 lose). It's effectively

template <typename T, typename ErrorCode>
class ResultOr {
  ErrorCode error;
  std::optional<T> value;
};

with a bunch of methods around it. Methods return a ResultOr object, so can return either a result or an error code, just via "return" statements, thanks to various ResultOr constructors. In the caller, it's then a runtime error to try to access "value" if it wasn't set (so this enforces checking the error code and dealing with it.)

Matt


On Thu, Jun 11, 2020 at 12:29 PM Larry Gritz <lg@...> wrote:
Oh, these code examples are extremely helpful.

I think Option 1 is inadequate, since it would preclude having the exception-free versions declared properly as 'noexcept'. I think that's an important aspect of this modernization (as well as adding 'noexcept' to the many functions that never had exceptions in the first place).

Aesthetically, I prefer option 2, though it has two acknowledged limitations: (a) On a case-by-case basis, we would need to agree on what would be returned in the error cases (are error-indicating values possible? what should they be?). (b) What to do if/when there are cases where more than one kind of error could occur, and if it's important for calling code to distinguish among them.

Option 3 handles those issues, but despite the precedent from std::filesystem, I think it's kind of ugly how it requires calling code to be littered with declarations of err_code variables, even if you have no intention of checking them (for example, if you know that the surrounding code makes the error condition impossible).

I can think of two other possible approaches (I'm merely listing them for consideration, not necessarily preferring them at this time):

4. Further template on whether we want exceptions (do I remember Kimball Thurston advocating this?).

5. To the extent that we end up with a header-only library, we could allow a preprocessor symbol (set by user code before #include of the Imath headers) to make a strong guarantee of exceptions or not:

#ifndef IMATH_EXCEPTIONS
#  define IMATH_EXCEPTIONS /* bikeshed color goes here */
#endif

#if IMATH_EXCEPTIONS
template<class T>
T Frustum<T>::aspect() const
{
    if (error case)
        throw ...
    return rightMinusLeft / topMinusBottom;
}
#else
template<class T>
T Frustum<T>::aspect() const noexcept
{
    if (error case)
        return T(0);
    return rightMinusLeft / topMinusBottom;
}
#endif


Just out of curiosity, have you done enough of an inventory to know how many functions total we are dealing with that throw exceptions now?


On Jun 11, 2020, at 11:53 AM, Owen T. via lists.aswf.io <ownthmpsn=protonmail.ch@...> wrote:

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code, 
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17


--
Larry Gritz








Larry Gritz
 

VFXPlatform 2021 is scheduled to be C++17.

While I can imagine some uses for exotic optional return types, I don't think that core math functions is where I'd want them to be.
I want my code using Imath to be as straight-line as possible, and my own preference is just to have "safe" versions where needed (as an example of what I mean, a "safe_sqrt" would clamp its input to minimum of 0 rather than have exceptions or error codes at all -- if somebody cares about identifying bad domain, they should check it before making the call).


On Jun 11, 2020, at 1:58 PM, Thiago Ize <thiago.ize@...> wrote:

std::optional, like most fancy c++ features, is neat on paper, and I'll probably find uses for it once I'm allowed to use C++17, but I get worried it'll still bring too much baggage. For instance, if you use the optional's value(), it'll cause the compiler to generate exception handling code (looking at the assembly I see a std::bad_optional_access can be thrown). If you use value_or() you can avoid the exceptions, which is nice, but you then need to supply the value to use when there's an error which can be either neat or annoying depending on the situation. Even when there's no error, both versions will never be as fast as code that does no error checking (not sure if that matters for imath).

For those curious in trying it out, here's my quick hack at implementing Matt's suggestion: https://gcc.godbolt.org/z/ZYFaap 

Also, the vfx reference platform for 2020 looks to be c++14 (no mention of what will be in 2021), so that might be another impediment even if this is the cleanest approach.

Thiago

On Thu, Jun 11, 2020 at 1:43 PM Matt Pharr <matt.pharr@...> wrote:
Drive-by: I've seen a nice idiom for this sort of thing in an exception-free codebase that also still gives you error codes (which options 1, 2, and 5 lose). It's effectively

template <typename T, typename ErrorCode>
class ResultOr {
  ErrorCode error;
  std::optional<T> value;
};

with a bunch of methods around it. Methods return a ResultOr object, so can return either a result or an error code, just via "return" statements, thanks to various ResultOr constructors. In the caller, it's then a runtime error to try to access "value" if it wasn't set (so this enforces checking the error code and dealing with it.)

Matt


On Thu, Jun 11, 2020 at 12:29 PM Larry Gritz <lg@...> wrote:
Oh, these code examples are extremely helpful.

I think Option 1 is inadequate, since it would preclude having the exception-free versions declared properly as 'noexcept'. I think that's an important aspect of this modernization (as well as adding 'noexcept' to the many functions that never had exceptions in the first place).

Aesthetically, I prefer option 2, though it has two acknowledged limitations: (a) On a case-by-case basis, we would need to agree on what would be returned in the error cases (are error-indicating values possible? what should they be?). (b) What to do if/when there are cases where more than one kind of error could occur, and if it's important for calling code to distinguish among them.

Option 3 handles those issues, but despite the precedent from std::filesystem, I think it's kind of ugly how it requires calling code to be littered with declarations of err_code variables, even if you have no intention of checking them (for example, if you know that the surrounding code makes the error condition impossible).

I can think of two other possible approaches (I'm merely listing them for consideration, not necessarily preferring them at this time):

4. Further template on whether we want exceptions (do I remember Kimball Thurston advocating this?).

5. To the extent that we end up with a header-only library, we could allow a preprocessor symbol (set by user code before #include of the Imath headers) to make a strong guarantee of exceptions or not:

#ifndef IMATH_EXCEPTIONS
#  define IMATH_EXCEPTIONS /* bikeshed color goes here */
#endif

#if IMATH_EXCEPTIONS
template<class T>
T Frustum<T>::aspect() const
{
    if (error case)
        throw ...
    return rightMinusLeft / topMinusBottom;
}
#else
template<class T>
T Frustum<T>::aspect() const noexcept
{
    if (error case)
        return T(0);
    return rightMinusLeft / topMinusBottom;
}
#endif


Just out of curiosity, have you done enough of an inventory to know how many functions total we are dealing with that throw exceptions now?


On Jun 11, 2020, at 11:53 AM, Owen T. via lists.aswf.io <ownthmpsn=protonmail.ch@...> wrote:

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code, 
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17


--
Larry Gritz







--
Larry Gritz




Nick Porcino
 

Thanks Owen!

I'm going to take an opinion here, attempting to take into account the foregoing discussion, and introducing my own biases and prejudices:

As Larry points out, option 1 isn't suitable as it does not allow elision of the throwing code.

Option 2 is necessary, because as Peter points out, our ideal case per our current thinking, is that existing code continues to compile without modification.

I prefer that we introduce unambiguous result codes. Not fancy modern C++ result codes, but straight forward comparable values lacking exotic semantics. I further suggest that we do not muddy things by contriving an Imath uber set of codes designed to capture all possible exceptional values, but rather we target results strictly and specifically. If an invert operation can succeed due to the matrix being invertible, or fail due to being singular, then we restrict the possible codes unambiguously:

enum class InvertResultCode { Invertible, Singular };

Next, we do not supply a return by reference in the new functions, but only an InvertResultCode in the case of an in place inversion, or an InvertResult in the const versions. The InvertResult is scoped to the class. This approach is similar to Matt's drive by suggestion, except that the value in the InvertResult is not guarded by a code affordance, just common sense. If the result is InvertResultCode is singular, I think we should simply memcpy the matrix into the result, because we should not leave the door open to UB due to uninitialized memory. std::optional should not be considered in this case, as it can throw exceptions, and has a number of other well documented overheads, as Thiago points out.

@@ -920,6 +935,19 @@ template <class T> class Matrix44
 
     Matrix44<T>         gjInverse (bool singExc = false) const;
 
+    // if code is Singular, result will be a copy of the matrix
+    struct InvertResult { InvertResultCode code, Matrix44 result };
+
+    // if the matrix is singular, no modification is made
+    InvertResultCode    invert () noexcept;
+
+    InvertResult        inverse () const noexcept;
+
+    // if the matrix is singular, no modification is made
+    InvertResultCode    gjInvert () noexcept;
+
+    InvertResult        gjInverse () const noexcept;
 
     //------------------------------------------------
     // Calculate the matrix minor of the (r,c) element

On Larry's reference of Kimball's suggestion, or was it mine? of an error handling policy object, I recommend we do not do that. I was a proponent of that approach for a long time, but my experience since then has been that it imposes an unnecessary level of genericity where it is more or less not warranted. As Larry speculates for the filesystem pattern, I found that a policy template trait litters and clutters the code with accommodations to its needs. 

To Larry's point 5 on a preprocessor symbol, I believe we should be invoking an option in the cmake file, and emitting the symbol in the ImathConfig.h file. I advocate a straight ifndef to guard the exception throwing code, as opposed to #if as I personally find #ifndef to be less error prone. I am flexible on this point.

Peter, you say - 

> Ideally, the solution should support libraries that want to compile against Imath, where those libraries want to offer the choice of throwing and no-throwing versions. 

Yes, I agree. You then say -

> Differently named functions would make that tedious, as would a compile-time preprocessor flag in Imath.

I respectfully disagree. I think that it is best to be explicit, where an API always behaves as defined, and does not have hidden behavioral changes due to compilation choices. That means, in my mind, that differently named functions are not tedious, but explicit, and allow me to express my intent in the code explicitly. You then go on to say,

>  It may also be that folks want exception handling on for a debug build only, and turn them off for an optimized release.

A combination of a preprocessor guard, and explicit intentional naming of all variants, means that one may combine behaviors to your heart's content, however, the suggested scenario of throwing in debug, and not in release becomes admittedly tedious. I propose that behavior that differs in runtime between debug and release builds is not in general desirable, as it has already bit us in the test suite. I am therefore in favor of this scenario being in fact, tedious, as it will be an intentional choice in code, not a hidden choice in a makefile.

Your suggestion in the gist of a templated parameter to select throwing or non-throwing code is elegant, clean reading, and exactly expresses intent, all of which are points that strongly support. Unfortunately, I think we can't actually do that, first because it means existing code does have to change to accomodate the API, and second because Microsoft compilers do not deadstrip symbols or code when a templated class is instantiated and exported from a DLL. All methods are instantiated and code generated irrespective of whether the compiler can discover a usage of the method, so that would be contradiction to our desire to generate code that strictly has no exception code in it. Arguably, we may decide to state that Imath is a static library, but it does not sidestep the issue as the Imath types may be embedded in a user class that is exported from a shared library, and the can of worms then reappears.

As to whether it's a requirement that Imath run quickly, the answer is yes, it should. I was at some pains years ago to structure the algorithms in exrenvmap such that it executed in a reasonable amount of time. I would prefer that we don't regress on that front in general. There are a great many other math libraries much faster than Imath, but at the same time, one shouldn't have to hesitate to use Imath because of a worry that it is unreasonably slow. Vectorizing compilers mean that we often get good results with Imath due its simplicity. As anecdata, I rewrote some inner loops at Oculus Research from Eigen to a combination of direct simd code and Imath code and got great speedups because the Eigen template instantiation was too complex for the compiler to do anything with, whereas the Imath code exactly matched canonical patterns that the vectorizing optimizer looks for.

- Nick


On Thu, Jun 11, 2020 at 3:12 PM Larry Gritz <lg@...> wrote:
VFXPlatform 2021 is scheduled to be C++17.

While I can imagine some uses for exotic optional return types, I don't think that core math functions is where I'd want them to be.
I want my code using Imath to be as straight-line as possible, and my own preference is just to have "safe" versions where needed (as an example of what I mean, a "safe_sqrt" would clamp its input to minimum of 0 rather than have exceptions or error codes at all -- if somebody cares about identifying bad domain, they should check it before making the call).


On Jun 11, 2020, at 1:58 PM, Thiago Ize <thiago.ize@...> wrote:

std::optional, like most fancy c++ features, is neat on paper, and I'll probably find uses for it once I'm allowed to use C++17, but I get worried it'll still bring too much baggage. For instance, if you use the optional's value(), it'll cause the compiler to generate exception handling code (looking at the assembly I see a std::bad_optional_access can be thrown). If you use value_or() you can avoid the exceptions, which is nice, but you then need to supply the value to use when there's an error which can be either neat or annoying depending on the situation. Even when there's no error, both versions will never be as fast as code that does no error checking (not sure if that matters for imath).

For those curious in trying it out, here's my quick hack at implementing Matt's suggestion: https://gcc.godbolt.org/z/ZYFaap 

Also, the vfx reference platform for 2020 looks to be c++14 (no mention of what will be in 2021), so that might be another impediment even if this is the cleanest approach.

Thiago

On Thu, Jun 11, 2020 at 1:43 PM Matt Pharr <matt.pharr@...> wrote:
Drive-by: I've seen a nice idiom for this sort of thing in an exception-free codebase that also still gives you error codes (which options 1, 2, and 5 lose). It's effectively

template <typename T, typename ErrorCode>
class ResultOr {
  ErrorCode error;
  std::optional<T> value;
};

with a bunch of methods around it. Methods return a ResultOr object, so can return either a result or an error code, just via "return" statements, thanks to various ResultOr constructors. In the caller, it's then a runtime error to try to access "value" if it wasn't set (so this enforces checking the error code and dealing with it.)

Matt


On Thu, Jun 11, 2020 at 12:29 PM Larry Gritz <lg@...> wrote:
Oh, these code examples are extremely helpful.

I think Option 1 is inadequate, since it would preclude having the exception-free versions declared properly as 'noexcept'. I think that's an important aspect of this modernization (as well as adding 'noexcept' to the many functions that never had exceptions in the first place).

Aesthetically, I prefer option 2, though it has two acknowledged limitations: (a) On a case-by-case basis, we would need to agree on what would be returned in the error cases (are error-indicating values possible? what should they be?). (b) What to do if/when there are cases where more than one kind of error could occur, and if it's important for calling code to distinguish among them.

Option 3 handles those issues, but despite the precedent from std::filesystem, I think it's kind of ugly how it requires calling code to be littered with declarations of err_code variables, even if you have no intention of checking them (for example, if you know that the surrounding code makes the error condition impossible).

I can think of two other possible approaches (I'm merely listing them for consideration, not necessarily preferring them at this time):

4. Further template on whether we want exceptions (do I remember Kimball Thurston advocating this?).

5. To the extent that we end up with a header-only library, we could allow a preprocessor symbol (set by user code before #include of the Imath headers) to make a strong guarantee of exceptions or not:

#ifndef IMATH_EXCEPTIONS
#  define IMATH_EXCEPTIONS /* bikeshed color goes here */
#endif

#if IMATH_EXCEPTIONS
template<class T>
T Frustum<T>::aspect() const
{
    if (error case)
        throw ...
    return rightMinusLeft / topMinusBottom;
}
#else
template<class T>
T Frustum<T>::aspect() const noexcept
{
    if (error case)
        return T(0);
    return rightMinusLeft / topMinusBottom;
}
#endif


Just out of curiosity, have you done enough of an inventory to know how many functions total we are dealing with that throw exceptions now?


On Jun 11, 2020, at 11:53 AM, Owen T. via lists.aswf.io <ownthmpsn=protonmail.ch@...> wrote:

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code, 
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17


--
Larry Gritz







--
Larry Gritz






--
--------------------------------
Nick Porcino @meshula
Virtual and augmented production, interactive applications, and robotics, since 1982

Kimball Thurston
 

Wow, you guys already had a healthy discussion and I haven't even had coffee yet.

What I had been think about was I think similar in some ways to all of these, yet perhaps a bit different / expanded, so I'll write it here as a mostly separate option. This has come up in a BoF or on the discussion list or somewhere before, but I was thinking it might be worth enabling the library to be a bit more flexible in it's representation at the same time we deal with the exceptions. This is in some ways conflating separate discussions, but imagine:


This allows us to have compile time switching of the storage to a simd type, as well as enabling / disabling the exceptions. Obviously there are all manner of conversion operators and such missing, and probably need to iterate on some of this and have some things like type traits to handle the ability to expand the type (i.e. float + float -> double if you want), and some other fiddly bits, but hopefully this gives a good flavor.

I do realize this changes the API, in that we would have to lose the storage option I wrote up to preserve v.x instead of v.x(), which is perhaps a portability issue. Although we might be able to handle that by rejiggering a bit to roll the storage down a level for the default type. Actually, that is probably a good idea to preserve the API behavior. meh, too lazy, we can do that if we like the idea.

However, the nugget of what I suggest is that it does allow you to have both types alive in the program at once, one throwing an exception, one not. And you can have a third or fourth "stack version" of vector that is implemented using simd, but leave the other as a tightly-packed / castable version that can be you big array of vertices on the heap, as they are simply multiple types. And I'd have to think about how to fix my above code, but we could also easily have in-place rebinding / conversion operations to treat one as another type.

Some places have patterns like normalize and normalizeExc (in the case of vector), while others have this function(bool doThrow) semantics. It seems like if we're cleaning things up, adding const / constexpr / whatever other c++11 cleanup, it might be worth cleaning up the API to be consistent, and I would argue that having separate types is the best way to do this rather than any compile time switch or separate functions. We can debate what the default type should be for V3f, although I would argue for the noexcept case.

So if the storage and math impl stuff is too much to swallow and this stuff should just stay dead simple, I can be convinced of that, although we could do a subset of that which is just the error handling, which I think is exactly what Nick and others are suggesting - just make the exception / no exception stuff a separate typed object, burdening the type system, and not the API.

K



On Fri, Jun 12, 2020 at 5:44 PM Nick Porcino <nick.porcino@...> wrote:
Thanks Owen!

I'm going to take an opinion here, attempting to take into account the foregoing discussion, and introducing my own biases and prejudices:

As Larry points out, option 1 isn't suitable as it does not allow elision of the throwing code.

Option 2 is necessary, because as Peter points out, our ideal case per our current thinking, is that existing code continues to compile without modification.

I prefer that we introduce unambiguous result codes. Not fancy modern C++ result codes, but straight forward comparable values lacking exotic semantics. I further suggest that we do not muddy things by contriving an Imath uber set of codes designed to capture all possible exceptional values, but rather we target results strictly and specifically. If an invert operation can succeed due to the matrix being invertible, or fail due to being singular, then we restrict the possible codes unambiguously:

enum class InvertResultCode { Invertible, Singular };

Next, we do not supply a return by reference in the new functions, but only an InvertResultCode in the case of an in place inversion, or an InvertResult in the const versions. The InvertResult is scoped to the class. This approach is similar to Matt's drive by suggestion, except that the value in the InvertResult is not guarded by a code affordance, just common sense. If the result is InvertResultCode is singular, I think we should simply memcpy the matrix into the result, because we should not leave the door open to UB due to uninitialized memory. std::optional should not be considered in this case, as it can throw exceptions, and has a number of other well documented overheads, as Thiago points out.

@@ -920,6 +935,19 @@ template <class T> class Matrix44
 
     Matrix44<T>         gjInverse (bool singExc = false) const;
 
+    // if code is Singular, result will be a copy of the matrix
+    struct InvertResult { InvertResultCode code, Matrix44 result };
+
+    // if the matrix is singular, no modification is made
+    InvertResultCode    invert () noexcept;
+
+    InvertResult        inverse () const noexcept;
+
+    // if the matrix is singular, no modification is made
+    InvertResultCode    gjInvert () noexcept;
+
+    InvertResult        gjInverse () const noexcept;
 
     //------------------------------------------------
     // Calculate the matrix minor of the (r,c) element

On Larry's reference of Kimball's suggestion, or was it mine? of an error handling policy object, I recommend we do not do that. I was a proponent of that approach for a long time, but my experience since then has been that it imposes an unnecessary level of genericity where it is more or less not warranted. As Larry speculates for the filesystem pattern, I found that a policy template trait litters and clutters the code with accommodations to its needs. 

To Larry's point 5 on a preprocessor symbol, I believe we should be invoking an option in the cmake file, and emitting the symbol in the ImathConfig.h file. I advocate a straight ifndef to guard the exception throwing code, as opposed to #if as I personally find #ifndef to be less error prone. I am flexible on this point.

Peter, you say - 

> Ideally, the solution should support libraries that want to compile against Imath, where those libraries want to offer the choice of throwing and no-throwing versions. 

Yes, I agree. You then say -

> Differently named functions would make that tedious, as would a compile-time preprocessor flag in Imath.

I respectfully disagree. I think that it is best to be explicit, where an API always behaves as defined, and does not have hidden behavioral changes due to compilation choices. That means, in my mind, that differently named functions are not tedious, but explicit, and allow me to express my intent in the code explicitly. You then go on to say,

>  It may also be that folks want exception handling on for a debug build only, and turn them off for an optimized release.

A combination of a preprocessor guard, and explicit intentional naming of all variants, means that one may combine behaviors to your heart's content, however, the suggested scenario of throwing in debug, and not in release becomes admittedly tedious. I propose that behavior that differs in runtime between debug and release builds is not in general desirable, as it has already bit us in the test suite. I am therefore in favor of this scenario being in fact, tedious, as it will be an intentional choice in code, not a hidden choice in a makefile.

Your suggestion in the gist of a templated parameter to select throwing or non-throwing code is elegant, clean reading, and exactly expresses intent, all of which are points that strongly support. Unfortunately, I think we can't actually do that, first because it means existing code does have to change to accomodate the API, and second because Microsoft compilers do not deadstrip symbols or code when a templated class is instantiated and exported from a DLL. All methods are instantiated and code generated irrespective of whether the compiler can discover a usage of the method, so that would be contradiction to our desire to generate code that strictly has no exception code in it. Arguably, we may decide to state that Imath is a static library, but it does not sidestep the issue as the Imath types may be embedded in a user class that is exported from a shared library, and the can of worms then reappears.

As to whether it's a requirement that Imath run quickly, the answer is yes, it should. I was at some pains years ago to structure the algorithms in exrenvmap such that it executed in a reasonable amount of time. I would prefer that we don't regress on that front in general. There are a great many other math libraries much faster than Imath, but at the same time, one shouldn't have to hesitate to use Imath because of a worry that it is unreasonably slow. Vectorizing compilers mean that we often get good results with Imath due its simplicity. As anecdata, I rewrote some inner loops at Oculus Research from Eigen to a combination of direct simd code and Imath code and got great speedups because the Eigen template instantiation was too complex for the compiler to do anything with, whereas the Imath code exactly matched canonical patterns that the vectorizing optimizer looks for.

- Nick

On Thu, Jun 11, 2020 at 3:12 PM Larry Gritz <lg@...> wrote:
VFXPlatform 2021 is scheduled to be C++17.

While I can imagine some uses for exotic optional return types, I don't think that core math functions is where I'd want them to be.
I want my code using Imath to be as straight-line as possible, and my own preference is just to have "safe" versions where needed (as an example of what I mean, a "safe_sqrt" would clamp its input to minimum of 0 rather than have exceptions or error codes at all -- if somebody cares about identifying bad domain, they should check it before making the call).


On Jun 11, 2020, at 1:58 PM, Thiago Ize <thiago.ize@...> wrote:

std::optional, like most fancy c++ features, is neat on paper, and I'll probably find uses for it once I'm allowed to use C++17, but I get worried it'll still bring too much baggage. For instance, if you use the optional's value(), it'll cause the compiler to generate exception handling code (looking at the assembly I see a std::bad_optional_access can be thrown). If you use value_or() you can avoid the exceptions, which is nice, but you then need to supply the value to use when there's an error which can be either neat or annoying depending on the situation. Even when there's no error, both versions will never be as fast as code that does no error checking (not sure if that matters for imath).

For those curious in trying it out, here's my quick hack at implementing Matt's suggestion: https://gcc.godbolt.org/z/ZYFaap 

Also, the vfx reference platform for 2020 looks to be c++14 (no mention of what will be in 2021), so that might be another impediment even if this is the cleanest approach.

Thiago

On Thu, Jun 11, 2020 at 1:43 PM Matt Pharr <matt.pharr@...> wrote:
Drive-by: I've seen a nice idiom for this sort of thing in an exception-free codebase that also still gives you error codes (which options 1, 2, and 5 lose). It's effectively

template <typename T, typename ErrorCode>
class ResultOr {
  ErrorCode error;
  std::optional<T> value;
};

with a bunch of methods around it. Methods return a ResultOr object, so can return either a result or an error code, just via "return" statements, thanks to various ResultOr constructors. In the caller, it's then a runtime error to try to access "value" if it wasn't set (so this enforces checking the error code and dealing with it.)

Matt


On Thu, Jun 11, 2020 at 12:29 PM Larry Gritz <lg@...> wrote:
Oh, these code examples are extremely helpful.

I think Option 1 is inadequate, since it would preclude having the exception-free versions declared properly as 'noexcept'. I think that's an important aspect of this modernization (as well as adding 'noexcept' to the many functions that never had exceptions in the first place).

Aesthetically, I prefer option 2, though it has two acknowledged limitations: (a) On a case-by-case basis, we would need to agree on what would be returned in the error cases (are error-indicating values possible? what should they be?). (b) What to do if/when there are cases where more than one kind of error could occur, and if it's important for calling code to distinguish among them.

Option 3 handles those issues, but despite the precedent from std::filesystem, I think it's kind of ugly how it requires calling code to be littered with declarations of err_code variables, even if you have no intention of checking them (for example, if you know that the surrounding code makes the error condition impossible).

I can think of two other possible approaches (I'm merely listing them for consideration, not necessarily preferring them at this time):

4. Further template on whether we want exceptions (do I remember Kimball Thurston advocating this?).

5. To the extent that we end up with a header-only library, we could allow a preprocessor symbol (set by user code before #include of the Imath headers) to make a strong guarantee of exceptions or not:

#ifndef IMATH_EXCEPTIONS
#  define IMATH_EXCEPTIONS /* bikeshed color goes here */
#endif

#if IMATH_EXCEPTIONS
template<class T>
T Frustum<T>::aspect() const
{
    if (error case)
        throw ...
    return rightMinusLeft / topMinusBottom;
}
#else
template<class T>
T Frustum<T>::aspect() const noexcept
{
    if (error case)
        return T(0);
    return rightMinusLeft / topMinusBottom;
}
#endif


Just out of curiosity, have you done enough of an inventory to know how many functions total we are dealing with that throw exceptions now?


On Jun 11, 2020, at 11:53 AM, Owen T. via lists.aswf.io <ownthmpsn=protonmail.ch@...> wrote:

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code, 
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17


--
Larry Gritz







--
Larry Gritz






--
--------------------------------
Nick Porcino @meshula
Virtual and augmented production, interactive applications, and robotics, since 1982

Larry Gritz
 

IMHO, unless there is a massive difference in performance or flexibility that makes a practical difference to many people, we should always choose the simplest implementation that will be most easily understood by users of the library and developers who want to modify it.

A complex multi-level templating scheme is just a tax on future developers trying to understand the library.

For exceptions, I think the simplest thing to do is to preserve all the current names and behaviors, but for the small number of functions that currently throw exceptions, make a second version with an obvious name that does not throw (either a "safe" version that clamps or otherwise avoids error conditions entirely, or a version that returns an error code or message either through its return value or an optional pointer). Those new exception-free functions, as well as the existing functions that don't throw, should then be marked 'noexcept'.

For supporting SIMD, etc. (including more extensive linear algebra capabilities), I wonder if maybe the simplest route forward is to punt on providing an implementation, but more modestly, simply allow seamless interoperability with library-specific vector classes with the following sort of scheme:

* Define a "compatible class" V as one containing a "using element_type = ..." and a "const size_t N = ...", and can refer to elements .x, .y, .z of our scalar type T. (I'm guessing on the criteria and names; if a different choice makes us more compatible with std::array or other conventions, that's fine.)

* Make a template Imath::is_vec_compatible<V, T>() that checks these things for any class V, versus our Vec3<T>. And maybe a second more stringent is_vec_memory_compatible<> that also ensures that x, y, z are in adjacent memory locations, exactly matching the memory layout of our structure.

* In Imath::Vec3<T>, use templates and enable_if to produce constructors from any compatible type, and if possible explicit casts to any compatible type. This might look like (schematically, excuse if this isn't 100% correct C++ off the top of my head):

    template<class T>  class Vec3 {
        // conform to the compatibility convention ourself
        using element_type = T;
        const size_t N = 3;
        constexpr size_t size() const { return N; }

        // the data -- addressable either as .x, .y, .z, or with []
        union {
            struct { T x, y, z; };
            T[N] vals_;
        }
        const T& operator[](size_t i) const { return vals_[i]; }  // better than (&x)[i]
        T& operator[](size_t i) { return vals_[i]; }

        // our usual constructors...
        Vec3 (const T& x, const T& y, const T& z) : x(x), y(y), z(z) { }

        // compatibility constructor
        template<class V, ENABLE_IF(is_vec_compatible<V,Vec3>)>
        Vec3(const V& v) : x(v.x), y(v.y), z(v.z) { }

       // Note that this compatibility constructor subsumes our current 
       // template <class S> Vec3 (const Vec3<S> &v);

        // Hmmm... it might also be worth having something like
        template<class V, class T, ENABLE_IF(is_vec_memory_compatible<V, Vec3<T>>
        const Vec3<T>& Ref(const V& v) { return *reinterpret_cast<const Vec*>(&v)); }
    };

* We would strongly encourage other libraries that want to publish (or use internally) to conform to the same convention and be mutually constructable from an Imath::Vec3.

So this would make it really straightforward to have seamless interop with other libraries or apps who still prefer to use some custom vector implementation either internally or even in their public API, without user code needing to do all sorts of ugly casting. It would mostly just work. From an application source perspective, it would mostly mean that any of its dependent library's vectors could be passed to an API call from any other one with the right compatible casting templates. 



On Jun 12, 2020, at 4:41 PM, Kimball Thurston <kdt3rd@...> wrote:

Wow, you guys already had a healthy discussion and I haven't even had coffee yet.

What I had been think about was I think similar in some ways to all of these, yet perhaps a bit different / expanded, so I'll write it here as a mostly separate option. This has come up in a BoF or on the discussion list or somewhere before, but I was thinking it might be worth enabling the library to be a bit more flexible in it's representation at the same time we deal with the exceptions. This is in some ways conflating separate discussions, but imagine:


This allows us to have compile time switching of the storage to a simd type, as well as enabling / disabling the exceptions. Obviously there are all manner of conversion operators and such missing, and probably need to iterate on some of this and have some things like type traits to handle the ability to expand the type (i.e. float + float -> double if you want), and some other fiddly bits, but hopefully this gives a good flavor.

I do realize this changes the API, in that we would have to lose the storage option I wrote up to preserve v.x instead of v.x(), which is perhaps a portability issue. Although we might be able to handle that by rejiggering a bit to roll the storage down a level for the default type. Actually, that is probably a good idea to preserve the API behavior. meh, too lazy, we can do that if we like the idea.

However, the nugget of what I suggest is that it does allow you to have both types alive in the program at once, one throwing an exception, one not. And you can have a third or fourth "stack version" of vector that is implemented using simd, but leave the other as a tightly-packed / castable version that can be you big array of vertices on the heap, as they are simply multiple types. And I'd have to think about how to fix my above code, but we could also easily have in-place rebinding / conversion operations to treat one as another type.

Some places have patterns like normalize and normalizeExc (in the case of vector), while others have this function(bool doThrow) semantics. It seems like if we're cleaning things up, adding const / constexpr / whatever other c++11 cleanup, it might be worth cleaning up the API to be consistent, and I would argue that having separate types is the best way to do this rather than any compile time switch or separate functions. We can debate what the default type should be for V3f, although I would argue for the noexcept case.

So if the storage and math impl stuff is too much to swallow and this stuff should just stay dead simple, I can be convinced of that, although we could do a subset of that which is just the error handling, which I think is exactly what Nick and others are suggesting - just make the exception / no exception stuff a separate typed object, burdening the type system, and not the API.

K



On Fri, Jun 12, 2020 at 5:44 PM Nick Porcino <nick.porcino@...> wrote:
Thanks Owen!

I'm going to take an opinion here, attempting to take into account the foregoing discussion, and introducing my own biases and prejudices:

As Larry points out, option 1 isn't suitable as it does not allow elision of the throwing code.

Option 2 is necessary, because as Peter points out, our ideal case per our current thinking, is that existing code continues to compile without modification.

I prefer that we introduce unambiguous result codes. Not fancy modern C++ result codes, but straight forward comparable values lacking exotic semantics. I further suggest that we do not muddy things by contriving an Imath uber set of codes designed to capture all possible exceptional values, but rather we target results strictly and specifically. If an invert operation can succeed due to the matrix being invertible, or fail due to being singular, then we restrict the possible codes unambiguously:

enum class InvertResultCode { Invertible, Singular };

Next, we do not supply a return by reference in the new functions, but only an InvertResultCode in the case of an in place inversion, or an InvertResult in the const versions. The InvertResult is scoped to the class. This approach is similar to Matt's drive by suggestion, except that the value in the InvertResult is not guarded by a code affordance, just common sense. If the result is InvertResultCode is singular, I think we should simply memcpy the matrix into the result, because we should not leave the door open to UB due to uninitialized memory. std::optional should not be considered in this case, as it can throw exceptions, and has a number of other well documented overheads, as Thiago points out.

@@ -920,6 +935,19 @@ template <class T> class Matrix44
 
     Matrix44<T>         gjInverse (bool singExc = false) const;
 
+    // if code is Singular, result will be a copy of the matrix
+    struct InvertResult { InvertResultCode code, Matrix44 result };
+
+    // if the matrix is singular, no modification is made
+    InvertResultCode    invert () noexcept;
+
+    InvertResult        inverse () const noexcept;
+
+    // if the matrix is singular, no modification is made
+    InvertResultCode    gjInvert () noexcept;
+
+    InvertResult        gjInverse () const noexcept;
 
     //------------------------------------------------
     // Calculate the matrix minor of the (r,c) element

On Larry's reference of Kimball's suggestion, or was it mine? of an error handling policy object, I recommend we do not do that. I was a proponent of that approach for a long time, but my experience since then has been that it imposes an unnecessary level of genericity where it is more or less not warranted. As Larry speculates for the filesystem pattern, I found that a policy template trait litters and clutters the code with accommodations to its needs. 

To Larry's point 5 on a preprocessor symbol, I believe we should be invoking an option in the cmake file, and emitting the symbol in the ImathConfig.h file. I advocate a straight ifndef to guard the exception throwing code, as opposed to #if as I personally find #ifndef to be less error prone. I am flexible on this point.

Peter, you say - 

> Ideally, the solution should support libraries that want to compile against Imath, where those libraries want to offer the choice of throwing and no-throwing versions. 

Yes, I agree. You then say -

> Differently named functions would make that tedious, as would a compile-time preprocessor flag in Imath.

I respectfully disagree. I think that it is best to be explicit, where an API always behaves as defined, and does not have hidden behavioral changes due to compilation choices. That means, in my mind, that differently named functions are not tedious, but explicit, and allow me to express my intent in the code explicitly. You then go on to say,

>  It may also be that folks want exception handling on for a debug build only, and turn them off for an optimized release.

A combination of a preprocessor guard, and explicit intentional naming of all variants, means that one may combine behaviors to your heart's content, however, the suggested scenario of throwing in debug, and not in release becomes admittedly tedious. I propose that behavior that differs in runtime between debug and release builds is not in general desirable, as it has already bit us in the test suite. I am therefore in favor of this scenario being in fact, tedious, as it will be an intentional choice in code, not a hidden choice in a makefile.

Your suggestion in the gist of a templated parameter to select throwing or non-throwing code is elegant, clean reading, and exactly expresses intent, all of which are points that strongly support. Unfortunately, I think we can't actually do that, first because it means existing code does have to change to accomodate the API, and second because Microsoft compilers do not deadstrip symbols or code when a templated class is instantiated and exported from a DLL. All methods are instantiated and code generated irrespective of whether the compiler can discover a usage of the method, so that would be contradiction to our desire to generate code that strictly has no exception code in it. Arguably, we may decide to state that Imath is a static library, but it does not sidestep the issue as the Imath types may be embedded in a user class that is exported from a shared library, and the can of worms then reappears.

As to whether it's a requirement that Imath run quickly, the answer is yes, it should. I was at some pains years ago to structure the algorithms in exrenvmap such that it executed in a reasonable amount of time. I would prefer that we don't regress on that front in general. There are a great many other math libraries much faster than Imath, but at the same time, one shouldn't have to hesitate to use Imath because of a worry that it is unreasonably slow. Vectorizing compilers mean that we often get good results with Imath due its simplicity. As anecdata, I rewrote some inner loops at Oculus Research from Eigen to a combination of direct simd code and Imath code and got great speedups because the Eigen template instantiation was too complex for the compiler to do anything with, whereas the Imath code exactly matched canonical patterns that the vectorizing optimizer looks for.

- Nick

On Thu, Jun 11, 2020 at 3:12 PM Larry Gritz <lg@...> wrote:
VFXPlatform 2021 is scheduled to be C++17.

While I can imagine some uses for exotic optional return types, I don't think that core math functions is where I'd want them to be.
I want my code using Imath to be as straight-line as possible, and my own preference is just to have "safe" versions where needed (as an example of what I mean, a "safe_sqrt" would clamp its input to minimum of 0 rather than have exceptions or error codes at all -- if somebody cares about identifying bad domain, they should check it before making the call).


On Jun 11, 2020, at 1:58 PM, Thiago Ize <thiago.ize@...> wrote:

std::optional, like most fancy c++ features, is neat on paper, and I'll probably find uses for it once I'm allowed to use C++17, but I get worried it'll still bring too much baggage. For instance, if you use the optional's value(), it'll cause the compiler to generate exception handling code (looking at the assembly I see a std::bad_optional_access can be thrown). If you use value_or() you can avoid the exceptions, which is nice, but you then need to supply the value to use when there's an error which can be either neat or annoying depending on the situation. Even when there's no error, both versions will never be as fast as code that does no error checking (not sure if that matters for imath).

For those curious in trying it out, here's my quick hack at implementing Matt's suggestion: https://gcc.godbolt.org/z/ZYFaap 

Also, the vfx reference platform for 2020 looks to be c++14 (no mention of what will be in 2021), so that might be another impediment even if this is the cleanest approach.

Thiago

On Thu, Jun 11, 2020 at 1:43 PM Matt Pharr <matt.pharr@...> wrote:
Drive-by: I've seen a nice idiom for this sort of thing in an exception-free codebase that also still gives you error codes (which options 1, 2, and 5 lose). It's effectively

template <typename T, typename ErrorCode>
class ResultOr {
  ErrorCode error;
  std::optional<T> value;
};

with a bunch of methods around it. Methods return a ResultOr object, so can return either a result or an error code, just via "return" statements, thanks to various ResultOr constructors. In the caller, it's then a runtime error to try to access "value" if it wasn't set (so this enforces checking the error code and dealing with it.)

Matt


On Thu, Jun 11, 2020 at 12:29 PM Larry Gritz <lg@...> wrote:
Oh, these code examples are extremely helpful.

I think Option 1 is inadequate, since it would preclude having the exception-free versions declared properly as 'noexcept'. I think that's an important aspect of this modernization (as well as adding 'noexcept' to the many functions that never had exceptions in the first place).

Aesthetically, I prefer option 2, though it has two acknowledged limitations: (a) On a case-by-case basis, we would need to agree on what would be returned in the error cases (are error-indicating values possible? what should they be?). (b) What to do if/when there are cases where more than one kind of error could occur, and if it's important for calling code to distinguish among them.

Option 3 handles those issues, but despite the precedent from std::filesystem, I think it's kind of ugly how it requires calling code to be littered with declarations of err_code variables, even if you have no intention of checking them (for example, if you know that the surrounding code makes the error condition impossible).

I can think of two other possible approaches (I'm merely listing them for consideration, not necessarily preferring them at this time):

4. Further template on whether we want exceptions (do I remember Kimball Thurston advocating this?).

5. To the extent that we end up with a header-only library, we could allow a preprocessor symbol (set by user code before #include of the Imath headers) to make a strong guarantee of exceptions or not:

#ifndef IMATH_EXCEPTIONS
#  define IMATH_EXCEPTIONS /* bikeshed color goes here */
#endif

#if IMATH_EXCEPTIONS
template<class T>
T Frustum<T>::aspect() const
{
    if (error case)
        throw ...
    return rightMinusLeft / topMinusBottom;
}
#else
template<class T>
T Frustum<T>::aspect() const noexcept
{
    if (error case)
        return T(0);
    return rightMinusLeft / topMinusBottom;
}
#endif


Just out of curiosity, have you done enough of an inventory to know how many functions total we are dealing with that throw exceptions now?


On Jun 11, 2020, at 11:53 AM, Owen T. via lists.aswf.io <ownthmpsn=protonmail.ch@...> wrote:

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code, 
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17


--
Larry Gritz







--
Larry Gritz








-- 
--------------------------------
Nick Porcino @meshula
Virtual and augmented production, interactive applications, and robotics, since 1982




--
Larry Gritz




Peter Hillman
 

I agree about simplicity. It would be nice to keep headers easy to understand, user code easy to write, and compiler error messages short.

On exceptions: 'NoExc' versions of methods would be appropriate for IMathFrustum, which only has exception-throwing methods. However, there are already three other approaches for opting in or out of exceptions within the library:

  1. Vector normalize already has an 'Exc' version to opt-in, and a NonNull that doesn't check. The default is intended to be non-throwing - opposite to how IMathFrustum might work.
  2. Matrix invert functions have an optional bool to control exceptions, and exceptions default to off if not provided.
  3. Matrix checkForZeroScaleInRow does the same, but exceptions default to on if the bool isn't provided.
It would be possible to change Matrix::invert(bool exc=false) to just Matrix::invert(bool exc), then overload it with 'Matrix::invert() noexcept' to give a completely exception free implementation without breaking existing functionality, but I can't work out how to overload checkForScaleInRow with a noexcept version.

This might mean that some methods have an 'Exc' version, and others a 'NoExc' version. Others might use extra parameters to control behavior. Does that seem too inconsistent, or should every method that has a throwing version have two extra versions, a NoExc and an Exc, alongside the original method that's backwards compatible? That would mean there's a Frustrum::setNoExc as well as a Frustum::set and Frustum::setExc which are the same.

Actually, there's a gotcha with normalize(): it appears that VecX<int>::normalize() will throw when normalizing a vector with more than 1 non-zero element. That suggests that normalize() and normalizeNoExc() might have to be different, and normalize() can't be 'noexcept', unless there's a sneaky way to mark it 'noexcept' for real types but not integer types.

On 20/06/20 8:05 am, Larry Gritz wrote:
IMHO, unless there is a massive difference in performance or flexibility that makes a practical difference to many people, we should always choose the simplest implementation that will be most easily understood by users of the library and developers who want to modify it.

A complex multi-level templating scheme is just a tax on future developers trying to understand the library.

For exceptions, I think the simplest thing to do is to preserve all the current names and behaviors, but for the small number of functions that currently throw exceptions, make a second version with an obvious name that does not throw (either a "safe" version that clamps or otherwise avoids error conditions entirely, or a version that returns an error code or message either through its return value or an optional pointer). Those new exception-free functions, as well as the existing functions that don't throw, should then be marked 'noexcept'.

For supporting SIMD, etc. (including more extensive linear algebra capabilities), I wonder if maybe the simplest route forward is to punt on providing an implementation, but more modestly, simply allow seamless interoperability with library-specific vector classes with the following sort of scheme:

* Define a "compatible class" V as one containing a "using element_type = ..." and a "const size_t N = ...", and can refer to elements .x, .y, .z of our scalar type T. (I'm guessing on the criteria and names; if a different choice makes us more compatible with std::array or other conventions, that's fine.)

* Make a template Imath::is_vec_compatible<V, T>() that checks these things for any class V, versus our Vec3<T>. And maybe a second more stringent is_vec_memory_compatible<> that also ensures that x, y, z are in adjacent memory locations, exactly matching the memory layout of our structure.

* In Imath::Vec3<T>, use templates and enable_if to produce constructors from any compatible type, and if possible explicit casts to any compatible type. This might look like (schematically, excuse if this isn't 100% correct C++ off the top of my head):

    template<class T>  class Vec3 {
        // conform to the compatibility convention ourself
        using element_type = T;
        const size_t N = 3;
        constexpr size_t size() const { return N; }

        // the data -- addressable either as .x, .y, .z, or with []
        union {
            struct { T x, y, z; };
            T[N] vals_;
        }
        const T& operator[](size_t i) const { return vals_[i]; }  // better than (&x)[i]
        T& operator[](size_t i) { return vals_[i]; }

        // our usual constructors...
        Vec3 (const T& x, const T& y, const T& z) : x(x), y(y), z(z) { }

        // compatibility constructor
        template<class V, ENABLE_IF(is_vec_compatible<V,Vec3>)>
        Vec3(const V& v) : x(v.x), y(v.y), z(v.z) { }

       // Note that this compatibility constructor subsumes our current 
       // template <class S> Vec3 (const Vec3<S> &v);

        // Hmmm... it might also be worth having something like
        template<class V, class T, ENABLE_IF(is_vec_memory_compatible<V, Vec3<T>>
        const Vec3<T>& Ref(const V& v) { return *reinterpret_cast<const Vec*>(&v)); }
    };

* We would strongly encourage other libraries that want to publish (or use internally) to conform to the same convention and be mutually constructable from an Imath::Vec3.

So this would make it really straightforward to have seamless interop with other libraries or apps who still prefer to use some custom vector implementation either internally or even in their public API, without user code needing to do all sorts of ugly casting. It would mostly just work. From an application source perspective, it would mostly mean that any of its dependent library's vectors could be passed to an API call from any other one with the right compatible casting templates. 



On Jun 12, 2020, at 4:41 PM, Kimball Thurston <kdt3rd@...> wrote:

Wow, you guys already had a healthy discussion and I haven't even had coffee yet.

What I had been think about was I think similar in some ways to all of these, yet perhaps a bit different / expanded, so I'll write it here as a mostly separate option. This has come up in a BoF or on the discussion list or somewhere before, but I was thinking it might be worth enabling the library to be a bit more flexible in it's representation at the same time we deal with the exceptions. This is in some ways conflating separate discussions, but imagine:


This allows us to have compile time switching of the storage to a simd type, as well as enabling / disabling the exceptions. Obviously there are all manner of conversion operators and such missing, and probably need to iterate on some of this and have some things like type traits to handle the ability to expand the type (i.e. float + float -> double if you want), and some other fiddly bits, but hopefully this gives a good flavor.

I do realize this changes the API, in that we would have to lose the storage option I wrote up to preserve v.x instead of v.x(), which is perhaps a portability issue. Although we might be able to handle that by rejiggering a bit to roll the storage down a level for the default type. Actually, that is probably a good idea to preserve the API behavior. meh, too lazy, we can do that if we like the idea.

However, the nugget of what I suggest is that it does allow you to have both types alive in the program at once, one throwing an exception, one not. And you can have a third or fourth "stack version" of vector that is implemented using simd, but leave the other as a tightly-packed / castable version that can be you big array of vertices on the heap, as they are simply multiple types. And I'd have to think about how to fix my above code, but we could also easily have in-place rebinding / conversion operations to treat one as another type.

Some places have patterns like normalize and normalizeExc (in the case of vector), while others have this function(bool doThrow) semantics. It seems like if we're cleaning things up, adding const / constexpr / whatever other c++11 cleanup, it might be worth cleaning up the API to be consistent, and I would argue that having separate types is the best way to do this rather than any compile time switch or separate functions. We can debate what the default type should be for V3f, although I would argue for the noexcept case.

So if the storage and math impl stuff is too much to swallow and this stuff should just stay dead simple, I can be convinced of that, although we could do a subset of that which is just the error handling, which I think is exactly what Nick and others are suggesting - just make the exception / no exception stuff a separate typed object, burdening the type system, and not the API.

K



On Fri, Jun 12, 2020 at 5:44 PM Nick Porcino <nick.porcino@...> wrote:
Thanks Owen!

I'm going to take an opinion here, attempting to take into account the foregoing discussion, and introducing my own biases and prejudices:

As Larry points out, option 1 isn't suitable as it does not allow elision of the throwing code.

Option 2 is necessary, because as Peter points out, our ideal case per our current thinking, is that existing code continues to compile without modification.

I prefer that we introduce unambiguous result codes. Not fancy modern C++ result codes, but straight forward comparable values lacking exotic semantics. I further suggest that we do not muddy things by contriving an Imath uber set of codes designed to capture all possible exceptional values, but rather we target results strictly and specifically. If an invert operation can succeed due to the matrix being invertible, or fail due to being singular, then we restrict the possible codes unambiguously:

enum class InvertResultCode { Invertible, Singular };

Next, we do not supply a return by reference in the new functions, but only an InvertResultCode in the case of an in place inversion, or an InvertResult in the const versions. The InvertResult is scoped to the class. This approach is similar to Matt's drive by suggestion, except that the value in the InvertResult is not guarded by a code affordance, just common sense. If the result is InvertResultCode is singular, I think we should simply memcpy the matrix into the result, because we should not leave the door open to UB due to uninitialized memory. std::optional should not be considered in this case, as it can throw exceptions, and has a number of other well documented overheads, as Thiago points out.

@@ -920,6 +935,19 @@ template <class T> class Matrix44
 
     Matrix44<T>         gjInverse (bool singExc = false) const;
 
+    // if code is Singular, result will be a copy of the matrix
+    struct InvertResult { InvertResultCode code, Matrix44 result };
+
+    // if the matrix is singular, no modification is made
+    InvertResultCode    invert () noexcept;
+
+    InvertResult        inverse () const noexcept;
+
+    // if the matrix is singular, no modification is made
+    InvertResultCode    gjInvert () noexcept;
+
+    InvertResult        gjInverse () const noexcept;
 
     //------------------------------------------------
     // Calculate the matrix minor of the (r,c) element

On Larry's reference of Kimball's suggestion, or was it mine? of an error handling policy object, I recommend we do not do that. I was a proponent of that approach for a long time, but my experience since then has been that it imposes an unnecessary level of genericity where it is more or less not warranted. As Larry speculates for the filesystem pattern, I found that a policy template trait litters and clutters the code with accommodations to its needs. 

To Larry's point 5 on a preprocessor symbol, I believe we should be invoking an option in the cmake file, and emitting the symbol in the ImathConfig.h file. I advocate a straight ifndef to guard the exception throwing code, as opposed to #if as I personally find #ifndef to be less error prone. I am flexible on this point.

Peter, you say - 

> Ideally, the solution should support libraries that want to compile against Imath, where those libraries want to offer the choice of throwing and no-throwing versions. 

Yes, I agree. You then say -

> Differently named functions would make that tedious, as would a compile-time preprocessor flag in Imath.

I respectfully disagree. I think that it is best to be explicit, where an API always behaves as defined, and does not have hidden behavioral changes due to compilation choices. That means, in my mind, that differently named functions are not tedious, but explicit, and allow me to express my intent in the code explicitly. You then go on to say,

>  It may also be that folks want exception handling on for a debug build only, and turn them off for an optimized release.

A combination of a preprocessor guard, and explicit intentional naming of all variants, means that one may combine behaviors to your heart's content, however, the suggested scenario of throwing in debug, and not in release becomes admittedly tedious. I propose that behavior that differs in runtime between debug and release builds is not in general desirable, as it has already bit us in the test suite. I am therefore in favor of this scenario being in fact, tedious, as it will be an intentional choice in code, not a hidden choice in a makefile.

Your suggestion in the gist of a templated parameter to select throwing or non-throwing code is elegant, clean reading, and exactly expresses intent, all of which are points that strongly support. Unfortunately, I think we can't actually do that, first because it means existing code does have to change to accomodate the API, and second because Microsoft compilers do not deadstrip symbols or code when a templated class is instantiated and exported from a DLL. All methods are instantiated and code generated irrespective of whether the compiler can discover a usage of the method, so that would be contradiction to our desire to generate code that strictly has no exception code in it. Arguably, we may decide to state that Imath is a static library, but it does not sidestep the issue as the Imath types may be embedded in a user class that is exported from a shared library, and the can of worms then reappears.

As to whether it's a requirement that Imath run quickly, the answer is yes, it should. I was at some pains years ago to structure the algorithms in exrenvmap such that it executed in a reasonable amount of time. I would prefer that we don't regress on that front in general. There are a great many other math libraries much faster than Imath, but at the same time, one shouldn't have to hesitate to use Imath because of a worry that it is unreasonably slow. Vectorizing compilers mean that we often get good results with Imath due its simplicity. As anecdata, I rewrote some inner loops at Oculus Research from Eigen to a combination of direct simd code and Imath code and got great speedups because the Eigen template instantiation was too complex for the compiler to do anything with, whereas the Imath code exactly matched canonical patterns that the vectorizing optimizer looks for.

- Nick

On Thu, Jun 11, 2020 at 3:12 PM Larry Gritz <lg@...> wrote:
VFXPlatform 2021 is scheduled to be C++17.

While I can imagine some uses for exotic optional return types, I don't think that core math functions is where I'd want them to be.
I want my code using Imath to be as straight-line as possible, and my own preference is just to have "safe" versions where needed (as an example of what I mean, a "safe_sqrt" would clamp its input to minimum of 0 rather than have exceptions or error codes at all -- if somebody cares about identifying bad domain, they should check it before making the call).


On Jun 11, 2020, at 1:58 PM, Thiago Ize <thiago.ize@...> wrote:

std::optional, like most fancy c++ features, is neat on paper, and I'll probably find uses for it once I'm allowed to use C++17, but I get worried it'll still bring too much baggage. For instance, if you use the optional's value(), it'll cause the compiler to generate exception handling code (looking at the assembly I see a std::bad_optional_access can be thrown). If you use value_or() you can avoid the exceptions, which is nice, but you then need to supply the value to use when there's an error which can be either neat or annoying depending on the situation. Even when there's no error, both versions will never be as fast as code that does no error checking (not sure if that matters for imath).

For those curious in trying it out, here's my quick hack at implementing Matt's suggestion: https://gcc.godbolt.org/z/ZYFaap 

Also, the vfx reference platform for 2020 looks to be c++14 (no mention of what will be in 2021), so that might be another impediment even if this is the cleanest approach.

Thiago

On Thu, Jun 11, 2020 at 1:43 PM Matt Pharr <matt.pharr@...> wrote:
Drive-by: I've seen a nice idiom for this sort of thing in an exception-free codebase that also still gives you error codes (which options 1, 2, and 5 lose). It's effectively

template <typename T, typename ErrorCode>
class ResultOr {
  ErrorCode error;
  std::optional<T> value;
};

with a bunch of methods around it. Methods return a ResultOr object, so can return either a result or an error code, just via "return" statements, thanks to various ResultOr constructors. In the caller, it's then a runtime error to try to access "value" if it wasn't set (so this enforces checking the error code and dealing with it.)

Matt


On Thu, Jun 11, 2020 at 12:29 PM Larry Gritz <lg@...> wrote:
Oh, these code examples are extremely helpful.

I think Option 1 is inadequate, since it would preclude having the exception-free versions declared properly as 'noexcept'. I think that's an important aspect of this modernization (as well as adding 'noexcept' to the many functions that never had exceptions in the first place).

Aesthetically, I prefer option 2, though it has two acknowledged limitations: (a) On a case-by-case basis, we would need to agree on what would be returned in the error cases (are error-indicating values possible? what should they be?). (b) What to do if/when there are cases where more than one kind of error could occur, and if it's important for calling code to distinguish among them.

Option 3 handles those issues, but despite the precedent from std::filesystem, I think it's kind of ugly how it requires calling code to be littered with declarations of err_code variables, even if you have no intention of checking them (for example, if you know that the surrounding code makes the error condition impossible).

I can think of two other possible approaches (I'm merely listing them for consideration, not necessarily preferring them at this time):

4. Further template on whether we want exceptions (do I remember Kimball Thurston advocating this?).

5. To the extent that we end up with a header-only library, we could allow a preprocessor symbol (set by user code before #include of the Imath headers) to make a strong guarantee of exceptions or not:

#ifndef IMATH_EXCEPTIONS
#  define IMATH_EXCEPTIONS /* bikeshed color goes here */
#endif

#if IMATH_EXCEPTIONS
template<class T>
T Frustum<T>::aspect() const
{
    if (error case)
        throw ...
    return rightMinusLeft / topMinusBottom;
}
#else
template<class T>
T Frustum<T>::aspect() const noexcept
{
    if (error case)
        return T(0);
    return rightMinusLeft / topMinusBottom;
}
#endif


Just out of curiosity, have you done enough of an inventory to know how many functions total we are dealing with that throw exceptions now?


On Jun 11, 2020, at 11:53 AM, Owen T. via lists.aswf.io <ownthmpsn=protonmail.ch@...> wrote:

I received a request to introduce an exception free alternative to any Imath function which throws an exception currently. There are a few ways of doing this. If any of them seem superior let me know. Now I am inclined towards option 1 as it has already been used by other parts of Imath.

OPTION 1: Bool parameter (currently used by Matrix inversion functions in Imath, false by default)
template<class T>
T Frustum<T>::aspect(bool singExc) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom) &&
        singExc)
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

OPTION 2: Two differently named functions
template<class T>
T Frustum<T>::aspectExc() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        throw std::domain_error ("Bad viewing frustum: "
                               "aspect ratio cannot be computed.");
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

OPTION 3: Implementation of new 'exc_code'
template<class T>
T Frustum<T>::aspect(exc_code& ec) const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    if (abs(topMinusBottom) < 1 &&
        abs(rightMinusLeft) > limits<T>::max() * abs(topMinusBottom))
    {
        ec = ("Bad viewing frustum: "
                  "aspect ratio cannot be computed.");
    //this can be changed to contain other information depending on exc_code, 
    }

    return rightMinusLeft / topMinusBottom;
}

template<class T>
T Frustum<T>::aspect() const
{
    T rightMinusLeft = _right-_left;
    T topMinusBottom = _top-_bottom;

    return rightMinusLeft / topMinusBottom;
}

Option 3 is similar to the exception handling of std::filesystem::rename since C++17


--
Larry Gritz







--
Larry Gritz








-- 
--------------------------------
Nick Porcino @meshula
Virtual and augmented production, interactive applications, and robotics, since 1982




--
Larry Gritz