DM-14429: Implement RFC-460: move afw.geom content to new geom package #356
Conversation
|
|
||
| std::ostream &operator<<(std::ostream &os, lsst::afw::geom::AffineTransform const &transform); | ||
| using lsst::geom::AffineTransform; |
There was a problem hiding this comment.
Can you use [[deprecated]] here? The documentation is kind of unclear about whether it applies to using-declarations.
There was a problem hiding this comment.
I gave this a try with a small test program (I'll attach it to the JIRA issue), and at least on gcc 7.3.0, it looks like
[[deprecated("use lsst::geom::AffineTransform instead")]]
typedef lsst::geom::AffineTransform AffineTransform;
does exactly what we want, but I no variant of using will compile if it is preceded by a [[deprecated]] tag.
I also think that if we add [[deprecated]] here, we might want to just squash those warnings via pragmas or compiler options in existing packages we haven't tried to convert, and that sort of defeats the purpose of not trying to convert them. I'd love to somehow get a warning for new code only, but the goal here was explicitly to avoid breaking old code whenever possible, and adding lots of warnings is sort of a form of breakage.
There was a problem hiding this comment.
What? Not breaking code is the whole point of warnings (deprecation or otherwise). It will be nearly impossible to phase out the old API otherwise.
I couldn't parse the first half of your comment. Are you saying that a typedef can be marked deprecated but a using-declaration cannot?
There was a problem hiding this comment.
What? Not breaking code is the whole point of warnings (deprecation or otherwise).
That's fair, but...
It will be nearly impossible to phase out the old API otherwise.
... I simply don't think it'll be worthwhile to try to phase out the old API in old code in anything like the near future, and I don't want these warnings to obscure more important ones until we decide that's worthwhile.
I couldn't parse the first half of your comment. Are you saying that a typedef can be marked deprecated but a using-declaration cannot?
Yes, at least for gcc.
There was a problem hiding this comment.
Then that makes it impractical, since templates can't be typedefed. 😞
There was a problem hiding this comment.
Ah, good point. We do tend to use most of the particular templates we've moved on this ticket almost exclusively via typedefs to specific template instantiations, but it certainly limits the usefulness of [[deprecated]] for projects like this.
There was a problem hiding this comment.
@kfindeisen @TallJimbo I'm inclined to skip trying to use [[deprecated]]. We can convert packages as they come up and we can find out what is broken by removing the deprecated aliases and fixing what shows up.
r-owen
force-pushed
the
tickets/DM-14429
branch
7 times, most recently
from
4127a58
to
29a7221
Compare
There was a problem hiding this comment.
Looks good.
I only very quickly scanned the clang-format and "switch to new names in Python/C++" commits as I assumed they were entirely mechanical. Please let me know if there were in fact any judgement calls in those I should look into.
Do we need to worry about pickle backwards compatibility for C++ classes without their own subpackage (e.g. I think Box2I would have been pickled as lsst.afw.geom.box.Box2I, not lsst.afw.geom.Box2I)?
Move geom basics Angle, Box, Extent, Point, SpherePoint, AffineTransform and LinearTransform to the geom package, including C++ code, python code, test utilities and unit tests. Add C++ and python aliases for backwards compatibility.
instead of afw/geom aliases
No description provided.