DM-9937: Add noexcept specifiers to applicable methods in afw #366
Conversation
There was a problem hiding this comment.
Looks great! I had a couple small quibbles in inline comments, but the only big issue is methods here that are unconditional noexcept when they call conditionally noexcept Point and Extent constructors; I haven't tried to call all of those out in this sweep, because I think it'd be better to change geom.
I did also notice that a lot of those isPersistable lack override; that doesn't need to be in scope for this ticket, so I certainly won't insist, but given that you've done the work of finding them it might be good to clean up at the same time.
| @@ -61,7 +61,7 @@ class SpanPixelIterator : public boost::iterator_facade<SpanPixelIterator, lsst: | |||
|
|
|||
| void advance(int n) { _p.getX() += n; } | |||
|
|
|||
| bool equal(SpanPixelIterator const& other) const { | |||
| bool equal(SpanPixelIterator const& other) const noexcept { | |||
There was a problem hiding this comment.
If the getX and getY calls here are noexcept, they should be in increment, decrement, and advance, too, even if they're const overloads.
But also note DM-14992, which I just created after noticing the incorrect iterator tag for this class - I'm hope to do that soon, as it's a potentially serious problem, and that will probably involve this class being rewritten without boost::iterator_facade anyway.
There was a problem hiding this comment.
I thought that forward iterators must always be comparable (the ForwardIterator spec does not mention an equality domain, while InputIterator does), while iterator moves have cases where they're allowed to throw (e.g., the spec does not define what happens if you increment a past-the-end iterator).
However, the proposed ForwardIterator concept explicitly says that only comparison of iterators over the same sequence is defined. I guess that means we should remove noexcept here, whether or not DM-14992 is implemented.
There was a problem hiding this comment.
ForwardIterators definitely need to be equality comparable, and I don't think any kind of iterator can be comparable unless a domain is specified (see previous conversation on pointer comparisons).
In any case, my comment here was just saying that if equal can be noexcept given its implementation, then the other methods could, too, given their implementations. I wasn't thinking about whether any possible implementation would be noexcept, and on those grounds what you have here is fine with me. (Making them all noexcept or making none of them noexcept is also fine with me, as I actually don't think there is any reasonable implementation of this class that could throw when the iterator is moved, and I don't think iterator concepts require noexcept anywhere).
| @@ -836,7 +838,7 @@ class Chebyshev1Function2 : public BasePolynomialFunction2<ReturnT> { | |||
| /** | |||
| * Get x,y range | |||
| */ | |||
| lsst::geom::Box2D getXYRange() const { | |||
| lsst::geom::Box2D getXYRange() const noexcept { | |||
There was a problem hiding this comment.
Looks like the Box2D constructor used here is not noexcept (though I believe it could be).
In addition, the Point2D constructors used here are only conditionally noexcept, so this should be too, if you fix the Box issue - but I think it'd be better to static_assert against anything that could make the Point2D constructors throw instead, as per my comments on the geom PR.
There was a problem hiding this comment.
Point2D is noexcept (2 != Eigen::Dynamic, and double is nothrow-copyable). In fact, it's impossible to speak of a class as being conditionally noexcept, because there's nothing to condition on.
As for Box2D, I left that constructor non-noexcept because it could potentially throw when max > min (which is prevented in this case by Chebyshev1Function2's class invariant; the XY-range can always be represented as a valid Box2D). Though I guess you could argue that such a change in behavior would break backward-compatibility in ways other than unexpectedly throwing an exception...
There was a problem hiding this comment.
Yeah, on second thought I was being too pedantic. I'll update the Box constructors to guarantee that max > min never throws; I think that will leave Box2I overflow as the only failure mode.
kfindeisen
force-pushed
the
tickets/DM-9937
branch
2 times, most recently
from
e98c7d3
to
2c3ba56
Compare
I've left many currently non-throwing methods without noexcept because an exception is a plausible response to invalid input. In particular, I have not added noexcept to any SpanPixelIterator method whose behavior is allowed to be undefined (e.g., dereferencing or incrementing a non-dereferencable iterator).
I have not added noexcept to setters that take potentially invalid inputs, even if the current implementation performs no input validation.
This commit fixes the default constructor not initializing ArrayKey::_size and adds a (redundant) virtual specifier for the destructor.
Few methods were marked noexcept on Calib, which makes no effort to ensure it's in a valid state, and Filter, which does not have a clear design.
kfindeisen
force-pushed
the
tickets/DM-9937
branch
from
2c3ba56
to
2e70634
Compare
This PR adds
noexceptspecifiers to several key types inlsst::afw. I have omitted these specifiers in a couple of key cases:vectororstringby value.ImageBaseand classes that compose it or its subclasses); to be on the safe side, I left these classes untouched even when a method "obviously shouldn't" throwI was a bit surprised to conclude that
afw::table::FieldBase<T>andKeyBase<T>are unconditionally nothrow-constructible and nothrow-copyable; please confirm that neither they nor any reasonable subclass can ever have a constructor failure for anyT.