Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: afw
-
Labels:None
-
Team:External
Description
Sogo Mineo writes:
GCC 8.1 was released recently, and it told me the following mistake in AFW.
The mistake resides in the master branch, too.
diff -ur afw-6.2-hsc+1/src/detection/FootprintSet.cc afw-6.2-hsc+1.new/src/detection/FootprintSet.cc--- afw-6.2-hsc+1/src/detection/FootprintSet.cc 2018-04-18 14:36:08.000000000 +0900+++ afw-6.2-hsc+1.new/src/detection/FootprintSet.cc 2018-05-08 15:07:26.862308982 +0900@@ -876,7 +876,7 @@Startspan(geom::Span const *span, image::Mask<MaskPixelT> *mask, DIRECTION const dir);~Startspan() { delete _span; }- bool getSpan() { return _span; }+ std::shared_ptr<geom::Span const> getSpan() { return _span; }bool Stop() { return _stop; }DIRECTION getDirection() { return _direction; }
Attachments
Activity
This code is actually bad in many more ways, including a destructor that explicitly calls delete on a shared_ptr. That's the worst of it, but this little helper class should also be holding its Span by value and not using static variables to hold config parameters. Makes me very worried about the remainder of this file.
I messed up the Jenkins run (specified wrong branch), and Jim Bosch wants to contribute some more.
Paul Price
added a comment - I messed up the Jenkins run (specified wrong branch), and Jim Bosch wants to contribute some more. Good news is that all the really bad stuff was in code that was completely unused (templates only instantiated in blocks that are already #ifdef'd out). I've just removed that.
There are a few more things I'm still cleaning up in that file in code that is in active use. All that code is correct, so far, but I'm sure at least some of it emits warnings, and there's some very overzealous use of shared_ptr I'd like to trim down for efficiency reasons.
Paul Price, since I've overwritten all of your changes, I'm just sending it back to you for review.
Jenkins run is underway at https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/27914/pipeline.
Minor comment: I suggest using emplace_back instead of construct+push_back.
Paul Price
added a comment - Minor comment: I suggest using emplace_back instead of construct+ push_back . 
Nate Lust, would you please approve this simple fix?
Jenkins is currently running.
price@pap-laptop:~/LSST/afw (tickets/DM-14353=) $ git sub-patchcommit 101f82680639518531b3433126a1356813dc6405 (HEAD -> tickets/DM-14353, origin/tickets/DM-14353)Author: Paul Price <price@astro.princeton.edu>Date: Tue May 8 10:03:46 2018 -0400fix return type for Startspan::getSpanThanks to Sogo Mineo (NAOJ) who identified the bug through gcc 8.1 andprovided the fix.diff --git a/src/detection/FootprintSet.cc b/src/detection/FootprintSet.ccindex 1f5be5d78..3dd1a99bd 100644--- a/src/detection/FootprintSet.cc+++ b/src/detection/FootprintSet.cc@@ -876,7 +876,7 @@ public:Startspan(geom::Span const *span, image::Mask<MaskPixelT> *mask, DIRECTION const dir);~Startspan() { delete _span; }- bool getSpan() { return _span; }+ std::shared_ptr<geom::Span const> getSpan() const { return _span; }bool Stop() { return _stop; }DIRECTION getDirection() { return _direction; }