afw/DM-7736: Avoid signed/unsigned integer comparison warnings. #97
Conversation
| @@ -211,7 +211,7 @@ struct FieldBase< Array<U> > { | |||
|
|
|||
| template <typename Derived> | |||
| void setValueDeep(Element * p, ndarray::ExpressionBase<Derived> const & value) const { | |||
| if (value.template getSize<0>() != _size) { | |||
| if (value.template getSize<0>() != static_cast<std::size_t>(_size)) { | |||
There was a problem hiding this comment.
It might make more sense for _size to be size_t, rather than int. Or will this cause problems for ndarray::makeVector?
There was a problem hiding this comment.
The signature of this FieldBase instantiation's constructor needs to match that of another instantiation for which negative values at schema-definition time are used to indicate a variable-size array field (i.e. a different size for each record), so this needs to stay as a signed integer. Switching to 64-bits might be worthwhile, but I'd rather not do that on this ticket.
There was a problem hiding this comment.
I don't see why that requires that the internal member be int (you can even keep the check unchanged, since it refers to the constructor parameter), but ok, it's a deliberate decision.
There was a problem hiding this comment.
Your instincts weren't at all wrong; it was actually a historical decision more than a deliberate one, and I liked your proposal to switch enough that I started looking into doing it here before I realized there was a closely-related type I couldn't easily change. But it is more of a change than I'd like to do now. I'm planning to do a bit of refactoring of afw::table in the not-too-distance future, and I'll include another look at all of these types in that.
| @@ -720,7 +720,7 @@ static void findFootprints( | |||
| fp->addSpan(spans[i0]->y + row0, spans[i0]->x0 + col0, spans[i0]->x1 + col0); | |||
| } | |||
|
|
|||
| if (good && !(fp->getNpix() < npixMin)) { | |||
| if (good && !(fp->getNpix() < static_cast<std::size_t>(npixMin))) { | |||
There was a problem hiding this comment.
A bit illegible. Perhaps rephrase as fp->getNpix() >= static_cast<std::size_t>(npixMin) to at least get rid of one set of parentheses?
| @@ -133,7 +133,7 @@ ndarray::Array<T const,1,1> ArrayKey<T>::get(BaseRecord const & record) const { | |||
| template <typename T> | |||
| void ArrayKey<T>::set(BaseRecord & record, ndarray::Array<T const,1,1> const & value) const { | |||
| LSST_THROW_IF_NE( | |||
| value.template getSize<0>(), _size, | |||
| value.template getSize<0>(), static_cast<std::size_t>(_size), | |||
There was a problem hiding this comment.
Should _size be of type size_t?
There was a problem hiding this comment.
(same as above - it's probably a bit more important to have the same time as another field type for which negative values are meaningful)
TallJimbo
force-pushed
the
tickets/DM-7736
branch
from
4585956
to
2b95f6e
Compare
TallJimbo
force-pushed
the
tickets/DM-7736
branch
from
2b95f6e
to
2974c91
Compare
No description provided.