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.

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

Replace uses of bare assert with OPENSIM_ASSERT #3548

Merged
merged 13 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ own the property `path` of type `AbstractPath` instead of the `GeometryPath` unn
been added to these forces to provide access to concrete path types (e.g., `updPath<T>`). In `Ligament` and
`Blankevoort1991Ligament`, usages of `get_GeometryPath`, `upd_GeometryPath`, etc., need to be been updated to
`getGeometryPath`, `updGeometryPath`, etc., or a suitable alternative.
- Added `Assertion.h` and associated `OPENSIM_ASSERT*` macros (#3531)
- Replaced uses of `assert` with `OPENSIM_ASSERT`, so that assertion may be configured via cmake in the future, and
so that OpenSim (esp. debug builds) throw instead of terminating the process (#3531)

v4.4.1
======
Expand Down
3 changes: 2 additions & 1 deletion OpenSim/Actuators/CoordinateActuator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
//==============================================================================
// INCLUDES
//==============================================================================
#include <OpenSim/Common/Exception.h>
#include <OpenSim/Simulation/Model/Model.h>
#include <OpenSim/Simulation/Model/CoordinateSet.h>
#include <OpenSim/Simulation/Model/ForceSet.h>
Expand Down Expand Up @@ -206,7 +207,7 @@ void CoordinateActuator::computeForce( const SimTK::State& s,
double CoordinateActuator::
getSpeed( const SimTK::State& s) const
{
assert(_coord);
OPENSIM_ASSERT_FRMOBJ(_coord != nullptr);
return _coord->getSpeedValue(s);
};

Expand Down
3 changes: 2 additions & 1 deletion OpenSim/Auxiliary/auxiliaryTestMuscleFunctions.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
* limitations under the License. *
* -------------------------------------------------------------------------- */

#include "OpenSim/Common/Assertion.h"
#include "OpenSim/Simulation/Model/ActivationFiberLengthMuscle.h"

/** A debugging utility for investigating muscle equilibrium failures.
Expand All @@ -40,7 +41,7 @@ void reportTendonAndFiberForcesAcrossFiberLengths(const T& muscle,
{
// should only be using this utility for equilibrium muscles
// with a compliant tendon
assert(!muscle.get_ignore_tendon_compliance());
OPENSIM_ASSERT(!muscle.get_ignore_tendon_compliance());

SimTK::State s = state;

Expand Down
3 changes: 2 additions & 1 deletion OpenSim/Common/AbstractProperty.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
// INCLUDES
//============================================================================
#include "AbstractProperty.h"
#include "Assertion.h"
#include "Object.h"

#include <limits>
Expand Down Expand Up @@ -205,7 +206,7 @@ void AbstractProperty::writeToXMLParentElement(Xml::Element& parent) const {
if (!isOneObjectProperty()) {
// Concrete property will be represented by an Xml element of
// the form <propName> value(s) </propName>.
assert(!getName().empty());
OPENSIM_ASSERT(!getName().empty());
Xml::Element propElement(getName());
writeToXMLElement(propElement);
parent.insertNodeAfter(parent.node_end(), propElement);
Expand Down
6 changes: 3 additions & 3 deletions OpenSim/Common/AbstractProperty.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
* -------------------------------------------------------------------------- */

// INCLUDES
#include <assert.h>
#include "Assertion.h"
#include <string>
#include <typeinfo>
#include "osimCommonDLL.h"
Expand Down Expand Up @@ -79,13 +79,13 @@ class OSIMCOMMON_API AbstractProperty {
/** Require that the number of values n in the value list of this property
be in the range aMin <= n <= aMax. */
void setAllowableListSize(int aMin, int aMax)
{ assert(0 <= aMin && aMin <= aMax);
{ OPENSIM_ASSERT(0 <= aMin && aMin <= aMax);
_minListSize = aMin; _maxListSize = aMax; }

/** Require that the number of values n in the value list of this property
be exactly n=aNum values. **/
void setAllowableListSize(int aNum)
{ assert(aNum >= 1); _minListSize = _maxListSize = aNum; }
{ OPENSIM_ASSERT(aNum >= 1); _minListSize = _maxListSize = aNum; }

// Default copy constructor and copy assignment operator.

Expand Down
18 changes: 18 additions & 0 deletions OpenSim/Common/Assertion.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#include "Assertion.h"

#include <OpenSim/Common/Exception.h>

void OpenSim::OnAssertionError(
char const* failingCode,
char const* failingFile,
char const* failingFunction,
unsigned int failingLine,
Object const* maybeSourceObject)
{
if (maybeSourceObject) {
throw Exception{failingFile, failingLine, failingFunction, *maybeSourceObject, failingCode};
}
else {
throw Exception{failingFile, failingLine, failingFunction, failingCode};
}
}
55 changes: 55 additions & 0 deletions OpenSim/Common/Assertion.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
#ifndef OPENSIM_ASSERTION_H_
#define OPENSIM_ASSERTION_H_

/* ------------------------------------------------------------------------- *
* OpenSim: Exception.h *
adamkewley marked this conversation as resolved.
Show resolved Hide resolved
* -------------------------------------------------------------------------- *
* The OpenSim API is a toolkit for musculoskeletal modeling and simulation. *
* See http://opensim.stanford.edu and the NOTICE file for more information. *
* OpenSim is developed at Stanford University and supported by the US *
* National Institutes of Health (U54 GM072970, R24 HD065690) and by DARPA *
* through the Warrior Web program. *
* *
* Copyright (c) 2005-2017 Stanford University and the Authors *
* Author(s): Frank C. Anderson *
* *
* Licensed under the Apache License, Version 2.0 (the "License"); you may *
* not use this file except in compliance with the License. You may obtain a *
* copy of the License at http://www.apache.org/licenses/LICENSE-2.0. *
* *
* Unless required by applicable law or agreed to in writing, software *
* distributed under the License is distributed on an "AS IS" BASIS, *
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. *
* See the License for the specific language governing permissions and *
* limitations under the License. *
* -------------------------------------------------------------------------- */

#include "osimCommonDLL.h"

namespace OpenSim {

class Object;

[[noreturn]] OSIMCOMMON_API void OnAssertionError(
char const* failingCode,
char const* failingFile,
char const* failingFunction,
unsigned int failingLine,
Object const* maybeSourceObject = nullptr
);
}

#define OPENSIM_ASSERT_ALWAYS(CONDITION) \
static_cast<bool>(CONDITION) ? static_cast<void>(0) : OpenSim::OnAssertionError(#CONDITION, __FILE__, __func__, __LINE__)
#define OPENSIM_ASSERT_FRMOBJ_ALWAYS(CONDITION) \
static_cast<bool>(CONDITION) ? static_cast<void>(0) : OpenSim::OnAssertionError(#CONDITION, __FILE__, __func__, __LINE__, this)

#if defined(NDEBUG)
#define OPENSIM_ASSERT(CONDITION)
#define OPENSIM_ASSERT_FRMOBJ(CONDITION)
#else
#define OPENSIM_ASSERT(CONDITION) OPENSIM_ASSERT_ALWAYS(CONDITION)
#define OPENSIM_ASSERT_FRMOBJ(CONDITION) OPENSIM_ASSERT_FRMOBJ_ALWAYS(CONDITION)
#endif

#endif // OPENSIM_ASSERTION_H_
3 changes: 2 additions & 1 deletion OpenSim/Common/DataTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ This file defines the DataTable_ class, which is used by OpenSim to provide an
in-memory container for data access and manipulation. */

#include "AbstractDataTable.h"
#include "Assertion.h"
#include "FileAdapter.h"
#include "SimTKcommon/internal/BigMatrix.h"
#include "SimTKcommon/internal/Quaternion.h"
Expand Down Expand Up @@ -959,7 +960,7 @@ class DataTable_ : public AbstractDataTable {
// get copy of labels
auto labels = getColumnLabels();

assert(labels.size() == _depData.ncol());
OPENSIM_ASSERT(labels.size() == _depData.ncol());

// shift columns unless we're already at the last column
for (size_t c = index; c < getNumColumns()-1; ++c) {
Expand Down
5 changes: 2 additions & 3 deletions OpenSim/Common/DebugUtilities.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@
#include "DebugUtilities.h"

#include "Logger.h"
#include <cassert>
#include <cstdlib>
#include <exception>
#include <fstream>
#include <iostream>
#include <sstream>
Expand All @@ -38,8 +38,7 @@ void Fatal_Error(const char* msg, const char* function, const char* file,
"line = {})", msg, function, file, line);
log_critical(str);
throw std::runtime_error(str);
assert(false);
exit(1);
std::terminate();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a drive-by edit, but is included here because:

  • The original code makes no logical sense anyway (the thread will stop on the throw)
  • The assert(false) is surplus, turns up in regex searches for bare asserts, and is compiler-toggleable
  • The exit (and my std::terminate) are also surplus, but I wasn't sure whether something funny could be going on (e.g. disabling exceptions or something unusual), so I just replaced the exit, with the more C++stdlib-friendly std::terminate()

}

/**
Expand Down
7 changes: 0 additions & 7 deletions OpenSim/Common/Exception.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
// INCLUDES
#include <iostream>
#include <string>
#include <cassert>
#include "osimCommonDLL.h"
#include "Exception.h"
#include "IO.h"
Expand Down Expand Up @@ -58,12 +57,6 @@ exception()
setMessage(aMsg);
_file = aFile;
_line = aLine;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also a drive-by edit that was made because I was using regex to flag uses of assert, and this one is likely to be a cute developer trick that was probably used a while ago (i.e. before IDEs like Visual Studio had debugger toggles to break on throwing an exception) but has been buried here.

// make it assert false when debugging...
//#ifndef NDEBUG
// print(cout);
// assert(false);
//#endif
}

namespace {
Expand Down
3 changes: 2 additions & 1 deletion OpenSim/Common/Object.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

#include "Object.h"

#include "Assertion.h"
#include "Exception.h"
#include "IO.h"
#include "Logger.h"
Expand Down Expand Up @@ -1663,7 +1664,7 @@ void Object::setObjectIsUpToDateWithProperties()

void Object::updateFromXMLDocument()
{
assert(_document != nullptr);
OPENSIM_ASSERT_FRMOBJ(_document != nullptr);

SimTK::Xml::Element e = _document->getRootDataElement();
IO::CwdChanger cwd = IO::CwdChanger::changeToParentOf(_document->getFileName());
Expand Down
8 changes: 4 additions & 4 deletions OpenSim/Common/Object.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@

// INCLUDES

#include "Assertion.h"
#include "osimCommonDLL.h"
#include "PropertySet.h"
#include "PropertyTable.h"
#include "Property.h"

#include <cstring>
#include <cassert>

// DISABLES MULTIPLE INSTANTIATION WARNINGS

Expand Down Expand Up @@ -1345,7 +1345,7 @@ ObjectProperty<T>::isEqualTo(const AbstractProperty& other) const {
// Property_Deprecated implementation can't copy this flag right.
if (this->getValueIsDefault() != other.getValueIsDefault())
return false;
assert(this->size() == other.size()); // base class checked
OPENSIM_ASSERT(this->size() == other.size()); // base class checked
const ObjectProperty& otherO = ObjectProperty::getAs(other);
for (int i=0; i<objects.size(); ++i) {
const T* const thisp = objects[i].get();
Expand Down Expand Up @@ -1404,11 +1404,11 @@ ObjectProperty<T>::readFromXMLElement

// Create an Object of the element tag's type.
Object* object = Object::newInstanceOfType(objTypeTag);
assert(object); // we just checked above
OPENSIM_ASSERT(object); // we just checked above
object->readObjectFromXMLNodeOrFile(*iter, versionNumber);

T* objectT = dynamic_cast<T*>(object);
assert(objectT); // should have worked by construction
OPENSIM_ASSERT(objectT); // should have worked by construction
adoptAndAppendValueVirtual(objectT); // don't copy
}

Expand Down
3 changes: 2 additions & 1 deletion OpenSim/Common/Property.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

// INCLUDES
#include "AbstractProperty.h"
#include "Assertion.h"
#include "Exception.h"
#include "Logger.h"

Expand Down Expand Up @@ -877,7 +878,7 @@ class SimpleProperty : public Property<T> {
// Property_Deprecated implementation can't copy this flag right.
if (this->getValueIsDefault() != other.getValueIsDefault())
return false;
assert(this->size() == other.size()); // base class checked
OPENSIM_ASSERT(this->size() == other.size()); // base class checked
const SimpleProperty& otherS = SimpleProperty::getAs(other);
for (int i=0; i<values.size(); ++i)
if (!Property<T>::TypeHelper::isEqual(values[i], otherS.values[i]))
Expand Down
5 changes: 3 additions & 2 deletions OpenSim/Common/PropertyDblVec.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#pragma warning( disable : 4251 )
#endif

#include "Assertion.h"
#include "osimCommonDLL.h"
#include <string>
#include "Property_Deprecated.h"
Expand Down Expand Up @@ -120,7 +121,7 @@ template<int M> class PropertyDblVec_ : public Property_Deprecated
using Property_Deprecated::setValue;
/** set value of this property from an array of doubles of equal or greater length */
void setValue(const Array<double> &anArray) override {
assert(anArray.getSize() >= M);
OPENSIM_ASSERT(anArray.getSize() >= M);
for(int i=0;i<M; i++)
_dblvec[i] = anArray[i];
}
Expand All @@ -130,7 +131,7 @@ template<int M> class PropertyDblVec_ : public Property_Deprecated
const SimTK::Vec<M>& getValueDblVec() const {return SimTK::Vec<M>::getAs(&_dblvec[0]); };
/** set value from double array */ // to be used by the serialization code
void setValue(int aSize, const double aArray[]) override {
assert(aSize == M);
OPENSIM_ASSERT(aSize == M);
setValue(SimTK::Vec<M>::getAs(aArray));
};
#ifndef SWIG
Expand Down
7 changes: 4 additions & 3 deletions OpenSim/Common/PropertyObj.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@


// INCLUDES
#include "Assertion.h"
#include "osimCommonDLL.h"
#include <string>
#include "Object.h"
Expand Down Expand Up @@ -74,11 +75,11 @@ class OSIMCOMMON_API PropertyObj : public Property_Deprecated
bool isAcceptableObjectTag
(const std::string& objectTypeTag) const override {return true;}
const Object& getValueAsObject(int index=-1) const override
{ assert(index <= 0); return getValueObj(); }
{ OPENSIM_ASSERT(index <= 0); return getValueObj(); }
Object& updValueAsObject(int index=-1) override
{ assert(index <= 0); return getValueObj(); }
{ OPENSIM_ASSERT(index <= 0); return getValueObj(); }
void setValueAsObject(const Object& obj, int index=-1) override
{ assert(index <= 0); delete _value; _value=obj.clone(); }
{ OPENSIM_ASSERT(index <= 0); delete _value; _value=obj.clone(); }

//--------------------------------------------------------------------------
// OPERATORS
Expand Down
7 changes: 4 additions & 3 deletions OpenSim/Common/PropertyObjPtr.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
* -------------------------------------------------------------------------- */

// INCLUDES
#include "Assertion.h"
#include "osimCommonDLL.h"
#include <string>
#include "Object.h"
Expand Down Expand Up @@ -88,11 +89,11 @@ template<class T=Object> class PropertyObjPtr : public Property_Deprecated
bool isAcceptableObjectTag
(const std::string& objectTypeTag) const override {return true;}
const Object& getValueAsObject(int index=-1) const override
{ assert(index <= 0); return *_value; }
{ OPENSIM_ASSERT(index <= 0); return *_value; }
Object& updValueAsObject(int index=-1) override
{ assert(index <= 0); return *_value; }
{ OPENSIM_ASSERT(index <= 0); return *_value; }
void setValueAsObject(const Object& obj, int index=-1) override
{ assert(index <= 0); setValue(obj.clone()); }
{ OPENSIM_ASSERT(index <= 0); setValue(obj.clone()); }

//--------------------------------------------------------------------------
// OPERATORS
Expand Down
6 changes: 4 additions & 2 deletions OpenSim/Common/PropertyTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
//============================================================================
#include "PropertyTable.h"

#include "Assertion.h"


using namespace OpenSim;
using namespace SimTK;
Expand Down Expand Up @@ -77,12 +79,12 @@ bool PropertyTable::equals(const PropertyTable& other) const {

int PropertyTable::adoptProperty(AbstractProperty* prop)
{
assert(prop);
OPENSIM_ASSERT(prop != nullptr);
const int nxtIndex = properties.size();
const std::string& name = prop->getName();

// Unnamed property should have had its Object class name used as its name.
assert(!name.empty());
OPENSIM_ASSERT(!name.empty());

if (hasProperty(name))
throw OpenSim::Exception
Expand Down
Loading
Loading