kcat / alure

Alure is a utility library for OpenAL, providing a C++ API and managing common tasks that include file loading, caching, and streaming
zlib License
70 stars 20 forks source link

Is std::out_of_range suitable for out of range floating points? #37

Closed McSinyx closed 4 years ago

McSinyx commented 4 years ago

From cppreference.com, std::out_of_range

reports errors that are consequence of attempt to access elements out of defined range.

Pitch and gain, for example, are floating points values, and thus IMHO should not throw this exception. I'd suggest std::domain_error which is defined as

may be used by the implementation to report domain errors, that is, situations where the inputs are outside of the domain on which an operation is defined.

Here is the list of the current throws of std::out_of_range:

$ ack out_of_range
src/auxeffectslot.cpp
56:        throw std::out_of_range("Gain out of range");

src/buffer.cpp
201:        throw std::out_of_range("Loop points out of range");
286:        throw std::out_of_range("Byte size result too large");

src/source.cpp
396:        throw std::out_of_range("Update length out of range");
398:        throw std::out_of_range("Queue size out of range");
517:        throw std::out_of_range("Fade gain target out of range");
519:        throw std::out_of_range("Fade duration out of range");
811:            throw std::out_of_range("Offset out of range");
977:        throw std::out_of_range("Pitch out of range");
989:        throw std::out_of_range("Gain out of range");
1000:        throw std::out_of_range("Gain range out of range");
1016:        throw std::out_of_range("Distance range out of range");
1181:        throw std::out_of_range("Cone angles out of range");
1196:        throw std::out_of_range("Outer cone gain out of range");
1213:        throw std::out_of_range("Rolloff factor out of range");
1229:        throw std::out_of_range("Doppler factor out of range");
1249:        throw std::out_of_range("Radius out of range");
1282:        throw std::out_of_range("Resampler index out of range");
1294:        throw std::out_of_range("Absorption factor out of range");
1374:        throw std::out_of_range("Gain value out of range");
1386:        throw std::out_of_range("Gain value out of range");
1448:        throw std::out_of_range("Gain value out of range");

src/context.cpp
817:        throw std::out_of_range("Async wake interval out of range");
1610:        throw std::out_of_range("Doppler factor out of range");
1620:        throw std::out_of_range("Speed of sound out of range");
1706:        throw std::out_of_range("Gain out of range");
1777:        throw std::out_of_range("Invalid meters per unit");

src/sourcegroup.cpp
128:        throw std::out_of_range("Gain out of range");
144:        throw std::out_of_range("Pitch out of range");

include/AL/alure2-typeviews.h
80:            throw std::out_of_range("alure::ArrayView::at: element out of range");
99:            throw std::out_of_range("alure::ArrayView::slice: pos out of range");
186:            throw std::out_of_range("alure::BasicStringView::substr: pos out of range");

I believe other than the ones thrown by type views, all should be substituted by a more proper exception.

Disclaimer: the reason I'm filing this issue is that Cython translate std::out_of_range as IndexError while std::domain_error is mapped to ValueError, which is more appropriate in Python context. I am not aware of the current usage of Alure 2 but I assume that backward compatibility might not be a major problem since you haven't made any official release of the new C++11 API.

kcat commented 4 years ago

Makes sense. Some may actually be better as std::invalid_argument, but for now I've changed the non-view-type std::out_of_range exceptions to std::domain_error, with commit 195e9347f0038bca9c27c2eeb7275efc1a2cdcf6.

I assume that backward compatibility might not be a major problem since you haven't made any official release of the new C++11 API.

Currently, Alure 2 makes no real promise to API/ABI compatibility. It will try to avoid any unnecessary or excessive changes, but given the C++ API and evolving functionality, it's not something that can be guaranteed.

McSinyx commented 4 years ago

Thank you for the quick response! I agree that for some cases, the exception might not exactly fall under std::domain_error and the more general std::invalid_argument might be a better choice, but I'm happy with the current state.