DM-33519: Add non-shared_ptr overloads to various FITS I/O functions. #632
Conversation
TallJimbo
force-pushed
the
tickets/DM-33519
branch
5 times, most recently
from
a354832
to
9715b4d
Compare
TallJimbo
force-pushed
the
tickets/DM-33519
branch
from
9715b4d
to
fad0837
Compare
| template <typename T> | ||
| [[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]] | ||
| ImageScale determine(image::ImageBase<T> const& image, | ||
| std::shared_ptr<image::Mask<image::MaskPixel> const> mask) const { |
There was a problem hiding this comment.
The original method had a nullptr default for mask; removing the default is a breaking change.
There was a problem hiding this comment.
I don't think it is, except maybe in some edge case involving member function pointers: the new overload is identical with this one up to this argument, and we now want that new one to be the one invoked when the defaulted argument is not provided by the caller. And if we did leave this argument defaulted as well, calls that do not pass at least one defaulted argument would be ambiguous (as the compiler reminded me after my first attempt left the defaults in).
| std::string const& mode = "w") const; | ||
| [[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]] | ||
| void writeFits(std::string const& fileName, | ||
| std::shared_ptr<daf::base::PropertySet const> metadata, |
There was a problem hiding this comment.
metadata originally had a nullptr default (same below).
| [[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]] | ||
| void writeFits(std::string const& filename, fits::ImageWriteOptions const& options, | ||
| std::string const& mode, | ||
| std::shared_ptr<daf::base::PropertySet const> header, |
There was a problem hiding this comment.
header originally had a nullptr default (same below).
| std::string const& mode = "w") const; | ||
| [[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]] | ||
| void writeFits(std::string const& fileName, | ||
| std::shared_ptr<daf::base::PropertySet const> metadata, |
There was a problem hiding this comment.
metadata originally had a nullptr default (same below).
| [[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]] | ||
| void writeFits(std::string const& filename, fits::ImageWriteOptions const& options, | ||
| std::string const& mode, | ||
| std::shared_ptr<daf::base::PropertySet const> header) const; |
There was a problem hiding this comment.
header originally had a nullptr default (same below).
| }, | ||
| "image"_a, | ||
| "mask"_a=nullptr | ||
| ); |
There was a problem hiding this comment.
Why is this lambda necessary, instead of using py::overload_cast to disambiguate which determine this is?
There was a problem hiding this comment.
I'm not sure; I tried the overload_cast version first (and I'm pretty sure I had it all right, including the easy-to-forget py::const_ argument), and it didn't work. I'll add a comment to that effect - I confess I do not have a good sense for which cases overload_cast can handle and which it cannot.
TallJimbo
force-pushed
the
tickets/DM-33519
branch
from
fad0837
to
2714959
Compare
The original versions of these functions violate rule 5-24b of the LSST C++ style guide and/or rule F7 of the C++ Core Guidelines: they take a smart pointer argument but don't actually need to, because they don't take ownership of the object or otherwise manipulate the object's lifetime. This is a viral problem, because it tempts downstream code to use smart pointers unnecessarily in arguments that will be forwarded to these. There are many more examples of this, but they'll be easier to resolve with or after RFC-840.
TallJimbo
force-pushed
the
tickets/DM-33519
branch
from
2714959
to
e3ebbba
Compare
No description provided.