Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DM-19690: audit for ast error-checking #52

Merged
merged 1 commit into from
May 16, 2019
Merged

DM-19690: audit for ast error-checking #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion include/astshim/CmpFrame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ class CmpFrame : public Frame {
explicit CmpFrame(Frame const &frame1, Frame const &frame2, std::string const &options = "")
: Frame(reinterpret_cast<AstFrame *>(astCmpFrame(const_cast<AstObject *>(frame1.getRawPtr()),
const_cast<AstObject *>(frame2.getRawPtr()),
"%s", options.c_str()))) {}
"%s", options.c_str()))) {
assertOK();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant because the Frame constructor calls the Mapping constructor which calls assertOK, and after that is called no AST code is executed that can set the flag. I admit that astIsA<x> is often called in these constructors, but:

  • I don't think that can set the OK flag false
  • Even if it can, this would only happen if it returned false, and at that point it's too late to call assertOK as an exception is already being raised

Where was the missing assertOK that triggered this bug? I think almost all the ones you added are redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not followed the code paths, but I put assertOK calls after any ast* calls (except for astIsA*). I figure it's better to be more careful than less careful, because it's a cheap test and if anything is missed, it manifests as a completely weird bug, and takes a full day of mucking around with gdb to understand it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good answer. OK. Go ahead.


virtual ~CmpFrame() {}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/CmpMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,9 @@ class CmpMap : public Mapping {
explicit CmpMap(Mapping const &map1, Mapping const &map2, bool series, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(astCmpMap(const_cast<AstObject *>(map1.getRawPtr()),
const_cast<AstObject *>(map2.getRawPtr()),
series, "%s", options.c_str()))) {}
series, "%s", options.c_str()))) {
assertOK();
}

virtual ~CmpMap() {}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/Frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,9 @@ class Frame : public Mapping {
@param[in] options Comma-separated list of attribute assignments.
*/
explicit Frame(int naxes, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(astFrame(naxes, "%s", options.c_str()))) {}
: Mapping(reinterpret_cast<AstMapping *>(astFrame(naxes, "%s", options.c_str()))) {
assertOK();
}

virtual ~Frame() {}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/FrameSet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@ class FrameSet : public Frame {
@param[in] options Comma-separated list of attribute assignments.
*/
explicit FrameSet(Frame const &frame, std::string const &options = "")
: FrameSet(astFrameSet(frame.copy()->getRawPtr(), "%s", options.c_str())) {}
: FrameSet(astFrameSet(frame.copy()->getRawPtr(), "%s", options.c_str())) {
assertOK();
}

/**
Construct a FrameSet from two frames and a mapping that connects them
Expand Down
4 changes: 3 additions & 1 deletion include/astshim/KeyMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ class KeyMap : public Object {
@param[in] options Comma-separated list of attribute assignments.
*/
explicit KeyMap(std::string const &options = "")
: Object(reinterpret_cast<AstObject *>(astKeyMap("%s", options.c_str()))) {}
: Object(reinterpret_cast<AstObject *>(astKeyMap("%s", options.c_str()))) {
assertOK();
}

virtual ~KeyMap(){};

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/LutMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ class LutMap : public Mapping {
*/
explicit LutMap(std::vector<double> const &lut, double start, double inc, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(
astLutMap(lut.size(), lut.data(), start, inc, "%s", options.c_str()))) {}
astLutMap(lut.size(), lut.data(), start, inc, "%s", options.c_str()))) {
assertOK();
}

virtual ~LutMap() {}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/MathMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,9 @@ class MathMap : public Mapping {
std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(astMathMap(nin, nout, fwd.size(), getCStrVec(fwd).data(),
rev.size(), getCStrVec(rev).data(), "%s",
options.c_str()))) {}
options.c_str()))) {
assertOK();
}

virtual ~MathMap() {}

Expand Down
8 changes: 6 additions & 2 deletions include/astshim/MatrixMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ class MatrixMap : public Mapping {
: Mapping(reinterpret_cast<AstMapping *>(
// form 0 = full matrix, 1 = diagonal elements only
astMatrixMap(matrix.getSize<1>(), matrix.getSize<0>(), 0, matrix.getData(), "%s",
options.c_str()))) {}
options.c_str()))) {
assertOK();
}

/**
Construct a MatrixMap from a 1-d vector of diagonal elements of a diagonal matrix
Expand All @@ -71,7 +73,9 @@ class MatrixMap : public Mapping {
explicit MatrixMap(std::vector<double> const &diag, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(
// form 0 = full matrix, 1 = diagonal elements only
astMatrixMap(diag.size(), diag.size(), 1, diag.data(), "%s", options.c_str()))) {}
astMatrixMap(diag.size(), diag.size(), 1, diag.data(), "%s", options.c_str()))) {
assertOK();
}

virtual ~MatrixMap() {}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/NormMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ class NormMap : public Mapping {
*/
explicit NormMap(Frame const &frame, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(
astNormMap(const_cast<AstObject *>(frame.getRawPtr()), "%s", options.c_str()))) {}
astNormMap(const_cast<AstObject *>(frame.getRawPtr()), "%s", options.c_str()))) {
assertOK();
}

virtual ~NormMap() {}

Expand Down
6 changes: 5 additions & 1 deletion include/astshim/Object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ class Object {
*/
static std::shared_ptr<Object> fromString(std::string const &str) {
auto *rawPtr = reinterpret_cast<AstObject *>(astFromString(str.c_str()));
assertOK(rawPtr);
return Object::_basicFromAstObject(rawPtr);
}

Expand Down Expand Up @@ -436,7 +437,10 @@ class Object {

@throws std::runtime_error if the attribute is read-only
*/
void set(std::string const &setting) { astSet(getRawPtr(), "%s", setting.c_str()); }
void set(std::string const &setting) {
astSet(getRawPtr(), "%s", setting.c_str());
assertOK();
}

/**
Set the value of an attribute as a bool
Expand Down
4 changes: 3 additions & 1 deletion include/astshim/PcdMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ class PcdMap : public Mapping {
os << "pcdcen.size() = " << pcdcen.size() << "; must be 2";
throw std::invalid_argument(os.str());
}
return astPcdMap(disco, pcdcen.data(), "%s", options.c_str());
auto result = astPcdMap(disco, pcdcen.data(), "%s", options.c_str());
assertOK();
return result;
}
};

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/RateMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ class RateMap : public Mapping {
*/
explicit RateMap(Mapping const &map, int ax1, int ax2, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(astRateMap(const_cast<AstObject *>(map.getRawPtr()), ax1,
ax2, "%s", options.c_str()))) {}
ax2, "%s", options.c_str()))) {
assertOK();
}

virtual ~RateMap() {}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/ShiftMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ class ShiftMap : public Mapping {
*/
explicit ShiftMap(std::vector<double> const &shift, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(
astShiftMap(shift.size(), shift.data(), "%s", options.c_str()))) {}
astShiftMap(shift.size(), shift.data(), "%s", options.c_str()))) {
assertOK();
}

virtual ~ShiftMap() {}

Expand Down
5 changes: 4 additions & 1 deletion include/astshim/SkyFrame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,9 @@ class SkyFrame : public Frame {
systems.
*/
explicit SkyFrame(std::string const &options = "")
: Frame(reinterpret_cast<AstFrame *>(astSkyFrame("%s", options.c_str()))) {}
: Frame(reinterpret_cast<AstFrame *>(astSkyFrame("%s", options.c_str()))) {
assertOK();
}

virtual ~SkyFrame() {}

Expand Down Expand Up @@ -235,6 +237,7 @@ class SkyFrame : public Frame {
*/
std::shared_ptr<Mapping> skyOffsetMap() {
auto *rawMap = reinterpret_cast<AstObject *>(astSkyOffsetMap(getRawPtr()));
assertOK();
return Object::fromAstObject<Mapping>(rawMap, false);
}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/SphMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ class SphMap : public Mapping {
@param[in] options Comma-separated list of attribute assignments.
*/
explicit SphMap(std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(astSphMap("%s", options.c_str()))) {}
: Mapping(reinterpret_cast<AstMapping *>(astSphMap("%s", options.c_str()))) {
assertOK();
}

virtual ~SphMap() {}

Expand Down
10 changes: 8 additions & 2 deletions include/astshim/TimeFrame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,9 @@ class TimeFrame : public Frame {
@param[in] options Comma-separated list of attribute assignments.
*/
explicit TimeFrame(std::string const &options = "")
: Frame(reinterpret_cast<AstFrame *>(astTimeFrame("%s", options.c_str()))) {}
: Frame(reinterpret_cast<AstFrame *>(astTimeFrame("%s", options.c_str()))) {
assertOK();
}

virtual ~TimeFrame() {}

Expand Down Expand Up @@ -118,7 +120,11 @@ class TimeFrame : public Frame {
since the epoch `00:00:00 UTC 1 January 1970 AD` (equivalent to TAI with a constant offset).
- Any inaccuracy in the system clock will be reflected in the value returned by this function.
*/
double currentTime() const { return detail::safeDouble(astCurrentTime(getRawPtr())); }
double currentTime() const {
auto result = detail::safeDouble(astCurrentTime(getRawPtr()));
assertOK();
return result;
}

/// Get @ref TimeFrame_AlignTimeScale "AlignTimeScale": time scale in which to align TimeFrames.
std::string getAlignTimeScale() const { return getC("AlignTimeScale"); }
Expand Down
4 changes: 3 additions & 1 deletion include/astshim/TimeMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ class TimeMap : public Mapping {
@param[in] options Comma-separated list of attribute assignments.
*/
explicit TimeMap(std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(astTimeMap(0, "%s", options.c_str()))) {}
: Mapping(reinterpret_cast<AstMapping *>(astTimeMap(0, "%s", options.c_str()))) {
assertOK();
}

virtual ~TimeMap() {}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/TranMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ class TranMap : public Mapping {
explicit TranMap(Mapping const &map1, Mapping const &map2, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(astTranMap(const_cast<AstObject *>(map1.getRawPtr()),
const_cast<AstObject *>(map2.getRawPtr()),
"%s", options.c_str()))) {}
"%s", options.c_str()))) {
assertOK();
}

virtual ~TranMap() {}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/UnitMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ class UnitMap : public Mapping {
@param[in] options Comma-separated list of attribute assignments.
*/
explicit UnitMap(int ncoord, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(astUnitMap(ncoord, "%s", options.c_str()))) {}
: Mapping(reinterpret_cast<AstMapping *>(astUnitMap(ncoord, "%s", options.c_str()))) {
assertOK();
}

virtual ~UnitMap() {}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/UnitNormMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ class UnitNormMap : public Mapping {
*/
explicit UnitNormMap(std::vector<double> const &centre, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(
astUnitNormMap(centre.size(), centre.data(), "%s", options.c_str()))) {}
astUnitNormMap(centre.size(), centre.data(), "%s", options.c_str()))) {
assertOK();
}

virtual ~UnitNormMap() {}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/WcsMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,9 @@ class WcsMap : public Mapping {
*/
explicit WcsMap(int ncoord, WcsType type, int lonax, int latax, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(
astWcsMap(ncoord, static_cast<int>(type), lonax, latax, "%s", options.c_str()))) {}
astWcsMap(ncoord, static_cast<int>(type), lonax, latax, "%s", options.c_str()))) {
assertOK();
}

virtual ~WcsMap() {}

Expand Down
6 changes: 4 additions & 2 deletions include/astshim/WinMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,10 @@ class WinMap : public Mapping {
os << "outb.size() = " << outb.size() << " != " << ncoord << " = ina.size()";
throw std::invalid_argument(os.str());
}
return astWinMap(static_cast<int>(ncoord), ina.data(), inb.data(), outa.data(), outb.data(), "%s",
options.c_str());
auto result = astWinMap(static_cast<int>(ncoord), ina.data(), inb.data(), outa.data(),
outb.data(), "%s", options.c_str());
assertOK();
return result;
}
};

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/XmlChan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ class XmlChan : public Channel {
explicit XmlChan(Stream &stream, std::string const &options = "")
: Channel(reinterpret_cast<AstChannel *>(
astXmlChan(detail::source, detail::sink, "%s", options.c_str())),
stream) {}
stream) {
assertOK();
}

virtual ~XmlChan() {}

Expand Down
4 changes: 3 additions & 1 deletion include/astshim/ZoomMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ class ZoomMap : public Mapping {
@param[in] options Comma-separated list of attribute assignments.
*/
explicit ZoomMap(int ncoord, double zoom, std::string const &options = "")
: Mapping(reinterpret_cast<AstMapping *>(astZoomMap(ncoord, zoom, "%s", options.c_str()))) {}
: Mapping(reinterpret_cast<AstMapping *>(astZoomMap(ncoord, zoom, "%s", options.c_str()))) {
assertOK();
}

virtual ~ZoomMap() {}

Expand Down
1 change: 1 addition & 0 deletions include/astshim/detail/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ static const int FITSLEN = 80;
inline void annulAstObject(AstObject *object) {
if (object != nullptr) {
astAnnul(object);
assertOK();
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/Channel.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
namespace ast {

Channel::Channel(Stream &stream, std::string const &options)
: Channel(astChannel(detail::source, detail::sink, "%s", options.c_str()), stream) {}
: Channel(astChannel(detail::source, detail::sink, "%s", options.c_str()), stream) {
assertOK();
}

Channel::Channel(AstChannel *chan, Stream &stream, bool isFits)
: Object(reinterpret_cast<AstObject *>(chan)), _stream(stream) {
Expand Down
9 changes: 6 additions & 3 deletions src/ChebyMap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,12 @@ AstChebyMap *ChebyMap::_makeRawChebyMap(ConstArray2D const &coeff_f, ConstArray2
"number of output axes");
}

return reinterpret_cast<AstChebyMap *>(astChebyMap(nin, nout, ncoeff_f, coeff_f.getData(), ncoeff_i,
coeff_i.getData(), lbnd_f.data(), ubnd_f.data(),
lbnd_i.data(), ubnd_i.data(), "%s", options.c_str()));
auto result = reinterpret_cast<AstChebyMap *>(astChebyMap(nin, nout, ncoeff_f, coeff_f.getData(),
ncoeff_i, coeff_i.getData(), lbnd_f.data(),
ubnd_f.data(), lbnd_i.data(), ubnd_i.data(),
"%s", options.c_str()));
assertOK();
return result;
}

/// Make a raw AstChebyMap with a specified forward transform and an optional iterative inverse.
Expand Down
5 changes: 4 additions & 1 deletion src/FitsChan.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,16 @@ char const *cstrOrNull(std::string const &str) { return str.empty() ? nullptr :
FitsChan::FitsChan(Stream &stream, std::string const &options)
: Channel(reinterpret_cast<AstChannel *>(
astFitsChan(detail::source, detail::sink, "%s", options.c_str())),
stream, true) {}
stream, true) {
assertOK();
}

FitsChan::~FitsChan() {
// when an astFitsChan is destroyed it first writes out any cards, but if I let astFitsChan
// do this automatically then it occurs while the Channel and its Source are being destroyed,
// which is too late
astWriteFits(getRawPtr());
// No 'assertOK' here: can't throw in a Dtor.
}

FoundValue<std::complex<double>> FitsChan::getFitsCF(std::string const &name,
Expand Down
5 changes: 4 additions & 1 deletion src/Mapping.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@ Array2D Mapping::linearApprox(PointD const &lbnd, PointD const &ubnd, double tol
detail::assertEqual(ubnd.size(), "ubnd.size", static_cast<std::size_t>(nIn), "nIn");
Array2D fit = ndarray::allocate(ndarray::makeVector(1 + nIn, nOut));
int isOK = astLinearApprox(getRawPtr(), lbnd.data(), ubnd.data(), tol, fit.getData());
assertOK();
if (!isOK) {
throw std::runtime_error("Mapping not sufficiently linear");
}
assertOK();
return fit;
}

Expand All @@ -74,6 +74,7 @@ std::shared_ptr<Class> Mapping::decompose(int i, bool copy) const {
AstMapping *rawMap2;
int series, invert1, invert2;
astDecompose(getRawPtr(), &rawMap1, &rawMap2, &series, &invert1, &invert2);
assertOK();

if (!rawMap2) {
// Not a compound object; free rawMap1 (rawMap2 is null, so no need to free it) and throw an exception
Expand All @@ -95,12 +96,14 @@ std::shared_ptr<Class> Mapping::decompose(int i, bool copy) const {
}
astAnnul(reinterpret_cast<AstObject *>(rawMap1));
astAnnul(reinterpret_cast<AstObject *>(rawMap2));
assertOK();

// If the mapping's internal invert flag does not match the value used when the CmpMap was made
// then invert the mapping. Note that it is not possible to create such objects in astshim
// but it is possible to read in objects created by other software.
if (invert != astGetI(retRawMap, "Invert")) {
astInvert(retRawMap);
assertOK();
}

return Object::fromAstObject<Class>(reinterpret_cast<AstObject *>(retRawMap), copy);
Expand Down