From b39e1067abcd8c76af9fd45a0d4a8f70b009cdfe Mon Sep 17 00:00:00 2001 From: Teemu Murtola Date: Sat, 8 Nov 2014 12:20:25 +0200 Subject: [PATCH] Improve selection error messages (use exceptions) Make all selection parsing error handling use exceptions for error reporting, and improve the understandability and context-sensitivity of the messages. Make the code in params.cpp exception-safe. Resolves #655 (except for some pending cleanup of now-unused code). Change-Id: If7bc1da91041232db23389c660cc2cd9c50addda --- src/gromacs/selection/keywords.h | 4 +- src/gromacs/selection/params.cpp | 566 +++++++++++++------------- src/gromacs/selection/parsetree.cpp | 68 +--- src/gromacs/selection/parsetree.h | 2 +- src/gromacs/selection/sm_keywords.cpp | 43 +- src/gromacs/selection/sm_same.cpp | 20 +- src/gromacs/utility/exceptions.cpp | 7 +- src/gromacs/utility/scoped_cptr.h | 11 +- 8 files changed, 356 insertions(+), 365 deletions(-) diff --git a/src/gromacs/selection/keywords.h b/src/gromacs/selection/keywords.h index 9401bd629c..eb503a7e7e 100644 --- a/src/gromacs/selection/keywords.h +++ b/src/gromacs/selection/keywords.h @@ -78,7 +78,7 @@ _gmx_selelem_set_kwstr_match_type(const gmx::SelectionTreeElementPointer &sel, gmx::SelectionStringMatchType matchType); /** Does custom processing for parameters of the \c same selection method. */ -int +void _gmx_selelem_custom_init_same(struct gmx_ana_selmethod_t **method, const gmx::SelectionParserParameterListPointer ¶ms, void *scanner); @@ -89,7 +89,7 @@ _gmx_selelem_custom_init_same(struct gmx_ana_selmethod_t **me * \param[in] method Keyword selection method to evaluate. * \param[in] child The group/positions to evaluate \p method in. * \param[in] scanner Scanner data structure. - * \returns Pointer to the created selection element (NULL on error). + * \returns Pointer to the created selection element. * * Creates a \ref SEL_EXPRESSION selection element that evaluates the keyword * method given by \p method in the group/positions given by \p child. diff --git a/src/gromacs/selection/params.cpp b/src/gromacs/selection/params.cpp index e1ac68d828..a246a6cb81 100644 --- a/src/gromacs/selection/params.cpp +++ b/src/gromacs/selection/params.cpp @@ -49,7 +49,7 @@ #include "gromacs/utility/cstringutil.h" #include "gromacs/utility/exceptions.h" #include "gromacs/utility/gmxassert.h" -#include "gromacs/utility/messagestringcollector.h" +#include "gromacs/utility/scoped_cptr.h" #include "gromacs/utility/smalloc.h" #include "gromacs/utility/stringutil.h" @@ -59,13 +59,7 @@ #include "selmethod.h" #include "selparam.h" -using gmx::SelectionLocation; -using gmx::SelectionParserValue; -using gmx::SelectionParserValueList; -using gmx::SelectionParserParameter; -using gmx::SelectionParserParameterList; -using gmx::SelectionTreeElement; -using gmx::SelectionTreeElementPointer; +using namespace gmx; /*! * \param[in] name Name of the parameter to search. @@ -118,15 +112,16 @@ gmx_ana_selparam_find(const char *name, int nparam, gmx_ana_selparam_t *param) * * \param[in,out] value Value to convert. * \param[in] type Type to convert to. + * \param[in] errors Errors will be reported into this as nested exceptions. * \param[in] scanner Scanner data structure. - * \returns 0 on success, a non-zero value on error. */ -static int -convert_value(SelectionParserValue *value, e_selvalue_t type, void *scanner) +static void +convert_value(SelectionParserValue *value, e_selvalue_t type, + ExceptionInitializer *errors, void *scanner) { if (value->type == type || type == NO_VALUE) { - return 0; + return; } if (value->hasExpressionValue()) { @@ -134,17 +129,21 @@ convert_value(SelectionParserValue *value, e_selvalue_t type, void *scanner) * reference positions. */ if (value->type == GROUP_VALUE && type == POS_VALUE) { - SelectionTreeElementPointer expr = - _gmx_sel_init_position(value->expr, NULL, scanner); - // FIXME: Use exceptions - if (!expr) + try { - return -1; + SelectionTreeElementPointer expr = + _gmx_sel_init_position(value->expr, NULL, scanner); + *value = SelectionParserValue::createExpr(expr); } - *value = SelectionParserValue::createExpr(expr); - return 0; + catch (UserInputError &ex) + { + std::string text(_gmx_sel_lexer_get_text(scanner, value->location())); + std::string context(formatString("In '%s'", text.c_str())); + ex.prependContext(context); + errors->addCurrentExceptionAsNested(); + } + return; } - return -1; } else { @@ -154,7 +153,7 @@ convert_value(SelectionParserValue *value, e_selvalue_t type, void *scanner) *value = SelectionParserValue::createRealRange(value->u.i.i1, value->u.i.i2, value->location()); - return 0; + return; } /* Reals that are integer-valued can also be converted */ if (value->type == REAL_VALUE && type == INT_VALUE) @@ -165,11 +164,16 @@ convert_value(SelectionParserValue *value, e_selvalue_t type, void *scanner) && gmx_within_tol(value->u.r.r2, i2, GMX_REAL_EPS)) { *value = SelectionParserValue::createIntegerRange(i1, i2, value->location()); - return 0; + return; } } } - return -1; + std::string text(_gmx_sel_lexer_get_text(scanner, value->location())); + std::string message( + formatString("Expression '%s' evaluates to a type is not valid in this context", + text.c_str())); + InvalidInputError ex(message); + errors->addNested(ex); } /*! \brief @@ -178,23 +182,20 @@ convert_value(SelectionParserValue *value, e_selvalue_t type, void *scanner) * \param[in,out] values Values to convert. * \param[in] type Type to convert to. * \param[in] scanner Scanner data structure. - * \returns 0 on success, a non-zero value on error. */ -static int +static void convert_values(SelectionParserValueList *values, e_selvalue_t type, void *scanner) { - int rc = 0; + ExceptionInitializer errors(""); SelectionParserValueList::iterator value; for (value = values->begin(); value != values->end(); ++value) { - int rc1 = convert_value(&*value, type, scanner); - if (rc1 != 0 && rc == 0) - { - rc = rc1; - } + convert_value(&*value, type, &errors, scanner); + } + if (errors.hasNestedExceptions()) + { + GMX_THROW(InvalidInputError(errors)); } - /* FIXME: More informative error messages */ - return rc; } /*! \brief @@ -297,40 +298,44 @@ cmp_real_range(const void *a, const void *b) * \param[in] values List of values. * \param param Parameter to parse. * \param[in] scanner Scanner data structure. - * \returns true if the values were parsed successfully, false otherwise. */ -static bool +static void parse_values_range(const SelectionParserValueList &values, gmx_ana_selparam_t *param, void *scanner) { - int *idata; - real *rdata; int i, j, n; param->flags &= ~SPAR_DYNAMIC; GMX_RELEASE_ASSERT(param->val.type == INT_VALUE || param->val.type == REAL_VALUE, "Invalid range parameter type"); - idata = NULL; - rdata = NULL; + int *idata = NULL; + real *rdata = NULL; + scoped_guard_sfree dataGuard; if (param->val.type == INT_VALUE) { snew(idata, values.size()*2); + dataGuard.reset(idata); } else { snew(rdata, values.size()*2); + dataGuard.reset(rdata); } i = 0; SelectionParserValueList::const_iterator value; for (value = values.begin(); value != values.end(); ++value) { + GMX_RELEASE_ASSERT(value->type == param->val.type, + "Invalid range value type (should have been caught earlier)"); if (value->hasExpressionValue()) { - _gmx_selparser_error(scanner, "expressions not supported within range parameters"); - return false; + std::string text(_gmx_sel_lexer_get_text(scanner, value->location())); + std::string message("Only simple values or 'A to B' ranges are " + "supported in this context"); + InvalidInputError ex(message); + ex.prependContext(formatString("Invalid expression '%s'", text.c_str())); + GMX_THROW(ex); } - GMX_RELEASE_ASSERT(value->type == param->val.type, - "Invalid range value type (should have been caught earlier)"); if (param->val.type == INT_VALUE) { int i1 = std::min(value->u.i.i1, value->u.i.i2); @@ -410,6 +415,7 @@ parse_values_range(const SelectionParserValueList &values, /* Store the values */ if (param->flags & SPAR_VARNUM) { + dataGuard.release(); param->val.nr = n; if (param->val.type == INT_VALUE) { @@ -426,20 +432,18 @@ parse_values_range(const SelectionParserValueList &values, { if (n != param->val.nr) { - _gmx_selparser_error(scanner, "the value should consist of exactly one range"); - sfree(idata); - sfree(rdata); - return false; + GMX_ASSERT(n == 1, + "Range parameters with a fixed count > 1 do not make sense"); + GMX_THROW(InvalidInputError("Only one value or 'A to B' range is " + "supported in this context")); } if (param->val.type == INT_VALUE) { memcpy(param->val.u.i, idata, 2*n*sizeof(int)); - sfree(idata); } else { memcpy(param->val.u.r, rdata, 2*n*sizeof(real)); - sfree(rdata); } } if (param->nvalptr) @@ -447,8 +451,6 @@ parse_values_range(const SelectionParserValueList &values, *param->nvalptr = param->val.nr; } param->nvalptr = NULL; - - return true; } /*! \brief @@ -458,12 +460,11 @@ parse_values_range(const SelectionParserValueList &values, * \param param Parameter to parse. * \param root Selection element to which child expressions are added. * \param[in] scanner Scanner data structure. - * \returns true if the values were parsed successfully, false otherwise. * * For integer ranges, the sequence of numbers from the first to second value * is stored, each as a separate value. */ -static bool +static void parse_values_varnum(const SelectionParserValueList &values, gmx_ana_selparam_t *param, const SelectionTreeElementPointer &root, @@ -473,7 +474,7 @@ parse_values_varnum(const SelectionParserValueList &values, param->flags &= ~SPAR_DYNAMIC; /* Compute number of values, considering also integer ranges. */ - size_t valueCount = values.size(); + int valueCount = static_cast(values.size()); if (param->val.type == INT_VALUE) { SelectionParserValueList::const_iterator value; @@ -490,7 +491,7 @@ parse_values_varnum(const SelectionParserValueList &values, if (param->val.type != INT_VALUE && param->val.type != REAL_VALUE && param->val.type != STR_VALUE && param->val.type != POS_VALUE) { - GMX_THROW(gmx::InternalError("Variable-count value type not implemented")); + GMX_THROW(InternalError("Variable-count value type not implemented")); } /* Reserve appropriate amount of memory */ @@ -504,18 +505,42 @@ parse_values_varnum(const SelectionParserValueList &values, { _gmx_selvalue_reserve(¶m->val, valueCount); } + /* Create a dummy child element to store the string values. + * This element is responsible for freeing the values, but carries no + * other function. */ + if (param->val.type == STR_VALUE) + { + SelectionTreeElementPointer child( + new SelectionTreeElement(SEL_CONST, SelectionLocation::createEmpty())); + _gmx_selelem_set_vtype(child, STR_VALUE); + child->setName(param->name); + child->flags &= ~SEL_ALLOCVAL; + child->flags |= SEL_FLAGSSET | SEL_VARNUMVAL | SEL_ALLOCDATA; + child->v.nr = valueCount; + _gmx_selvalue_setstore(&child->v, param->val.u.s); + /* Because the child is not group-valued, the u union is not used + * for anything, so we can abuse it by storing the parameter value + * as place_child() expects, but this is really ugly... */ + child->u.param = param; + place_child(root, child, param); + } + param->val.nr = valueCount; i = 0; SelectionParserValueList::const_iterator value; for (value = values.begin(); value != values.end(); ++value) { + GMX_RELEASE_ASSERT(value->type == param->val.type, + "Invalid value type (should have been caught earlier)"); if (value->hasExpressionValue()) { - _gmx_selparser_error(scanner, "expressions not supported within value lists"); - return false; + std::string text(_gmx_sel_lexer_get_text(scanner, value->location())); + std::string message("Selection expressions are not supported in this " + "context when multiple values are provided"); + InvalidInputError ex(message); + ex.prependContext(formatString("Invalid expression '%s'", text.c_str())); + GMX_THROW(ex); } - GMX_RELEASE_ASSERT(value->type == param->val.type, - "Invalid value type (should have been caught earlier)"); switch (param->val.type) { case INT_VALUE: @@ -537,8 +562,12 @@ parse_values_varnum(const SelectionParserValueList &values, case REAL_VALUE: if (value->u.r.r1 != value->u.r.r2) { - _gmx_selparser_error(scanner, "real ranges not supported"); - return false; + std::string text(_gmx_sel_lexer_get_text(scanner, value->location())); + std::string message + = formatString("Real range ('%s') is not supported in this context", + text.c_str()); + InvalidInputError ex(message); + GMX_THROW(ex); } param->val.u.r[i++] = value->u.r.r1; break; @@ -547,36 +576,16 @@ parse_values_varnum(const SelectionParserValueList &values, break; case POS_VALUE: copy_rvec(value->u.x, param->val.u.p->x[i++]); break; default: /* Should not be reached */ - GMX_THROW(gmx::InternalError("Variable-count value type not implemented")); + GMX_RELEASE_ASSERT(false, "Variable-count value type not implemented"); } } - param->val.nr = i; + GMX_RELEASE_ASSERT(i == valueCount, + "Inconsistent value count wrt. the actual value population"); if (param->nvalptr) { *param->nvalptr = param->val.nr; } param->nvalptr = NULL; - /* Create a dummy child element to store the string values. - * This element is responsible for freeing the values, but carries no - * other function. */ - if (param->val.type == STR_VALUE) - { - SelectionTreeElementPointer child( - new SelectionTreeElement(SEL_CONST, SelectionLocation::createEmpty())); - _gmx_selelem_set_vtype(child, STR_VALUE); - child->setName(param->name); - child->flags &= ~SEL_ALLOCVAL; - child->flags |= SEL_FLAGSSET | SEL_VARNUMVAL | SEL_ALLOCDATA; - child->v.nr = param->val.nr; - _gmx_selvalue_setstore(&child->v, param->val.u.s); - /* Because the child is not group-valued, the u union is not used - * for anything, so we can abuse it by storing the parameter value - * as place_child() expects, but this is really ugly... */ - child->u.param = param; - place_child(root, child, param); - } - - return true; } /*! \brief @@ -617,16 +626,23 @@ add_child(const SelectionTreeElementPointer &root, gmx_ana_selparam_t *param, child->u.param = param; if (child->v.type != param->val.type) { - _gmx_selparser_error(scanner, "invalid expression value"); - // FIXME: Use exceptions. - return SelectionTreeElementPointer(); + // TODO: It would be nice to say what is the expected type. + std::string text(_gmx_sel_lexer_get_text(scanner, expr->location())); + std::string message + = formatString("Expression '%s' is not valid in this context " + "(produces the wrong type of values)", + text.c_str()); + GMX_THROW(InvalidInputError(message)); } _gmx_selelem_update_flags(child); if ((child->flags & SEL_DYNAMIC) && !(param->flags & SPAR_DYNAMIC)) { - _gmx_selparser_error(scanner, "dynamic values not supported"); - // FIXME: Use exceptions. - return SelectionTreeElementPointer(); + std::string text(_gmx_sel_lexer_get_text(scanner, expr->location())); + std::string message + = formatString("Expression '%s' is dynamic, which is not " + "valid in this context", + text.c_str()); + GMX_THROW(InvalidInputError(message)); } if (!(child->flags & SEL_DYNAMIC)) { @@ -644,9 +660,8 @@ add_child(const SelectionTreeElementPointer &root, gmx_ana_selparam_t *param, * \param param Parameter to parse. * \param root Selection element to which child expressions are added. * \param[in] scanner Scanner data structure. - * \returns true if the values were parsed successfully, false otherwise. */ -static bool +static void parse_values_varnum_expr(const SelectionParserValueList &values, gmx_ana_selparam_t *param, const SelectionTreeElementPointer &root, @@ -657,10 +672,6 @@ parse_values_varnum_expr(const SelectionParserValueList &values, SelectionTreeElementPointer child = add_child(root, param, values.front().expr, scanner); - if (!child) - { - return false; - } /* Process single-valued expressions */ /* TODO: We should also handle SEL_SINGLEVAL expressions here */ @@ -674,13 +685,16 @@ parse_values_varnum_expr(const SelectionParserValueList &values, *param->nvalptr = param->val.nr; } param->nvalptr = NULL; - return true; + return; } if (!(child->flags & SEL_VARNUMVAL)) { - _gmx_selparser_error(scanner, "invalid expression value"); - return false; + std::string text(_gmx_sel_lexer_get_text(scanner, values.front().location())); + std::string message + = formatString("Expression '%s' is invalid in this context", + text.c_str()); + GMX_THROW(InvalidInputError(message)); } child->flags |= SEL_ALLOCVAL; @@ -688,8 +702,6 @@ parse_values_varnum_expr(const SelectionParserValueList &values, *param->nvalptr = param->val.nr; /* Rest of the initialization is done during compilation in * init_method(). */ - - return true; } /*! \brief @@ -705,14 +717,17 @@ parse_values_varnum_expr(const SelectionParserValueList &values, * as the value \p i of \p param. * This function is used internally by parse_values_std(). */ -static bool +static void set_expr_value_store(const SelectionTreeElementPointer &sel, gmx_ana_selparam_t *param, int i, void *scanner) { if (sel->v.type != GROUP_VALUE && !(sel->flags & SEL_SINGLEVAL)) { - _gmx_selparser_error(scanner, "invalid expression value"); - return false; + std::string text(_gmx_sel_lexer_get_text(scanner, sel->location())); + std::string message + = formatString("Expression '%s' is invalid in this context", + text.c_str()); + GMX_THROW(InvalidInputError(message)); } switch (sel->v.type) { @@ -722,11 +737,10 @@ set_expr_value_store(const SelectionTreeElementPointer &sel, case POS_VALUE: sel->v.u.p = ¶m->val.u.p[i]; break; case GROUP_VALUE: sel->v.u.g = ¶m->val.u.g[i]; break; default: /* Error */ - GMX_THROW(gmx::InternalError("Invalid value type")); + GMX_THROW(InternalError("Invalid value type")); } sel->v.nr = 1; sel->v.nalloc = -1; - return true; } /*! \brief @@ -736,12 +750,11 @@ set_expr_value_store(const SelectionTreeElementPointer &sel, * \param param Parameter to parse. * \param root Selection element to which child expressions are added. * \param[in] scanner Scanner data structure. - * \returns true if the values were parsed successfully, false otherwise. * * For integer ranges, the sequence of numbers from the first to second value * is stored, each as a separate value. */ -static bool +static void parse_values_std(const SelectionParserValueList &values, gmx_ana_selparam_t *param, const SelectionTreeElementPointer &root, void *scanner) @@ -754,17 +767,14 @@ parse_values_std(const SelectionParserValueList &values, { if (values.size() > 1) { - _gmx_selparser_error(scanner, "more than one value not supported"); - return false; + GMX_THROW(InvalidInputError( + "Only a single value or a single expression is " + "supported in this context")); } if (values.front().hasExpressionValue()) { SelectionTreeElementPointer child - = add_child(root, param, values.front().expr, scanner); - if (!child) - { - return false; - } + = add_child(root, param, values.front().expr, scanner); child->flags |= SEL_ALLOCVAL; if (child->v.type != GROUP_VALUE && (child->flags & SEL_ATOMVAL)) { @@ -776,7 +786,7 @@ parse_values_std(const SelectionParserValueList &values, { *param->nvalptr = -1; } - return true; + return; } param->flags &= ~SPAR_ATOMVAL; param->val.nr = 1; @@ -790,7 +800,8 @@ parse_values_std(const SelectionParserValueList &values, { _gmx_selvalue_reserve(¶m->val, 1); } - return set_expr_value_store(child, param, 0, scanner); + set_expr_value_store(child, param, 0, scanner); + return; } /* If we reach here, proceed with normal parameter handling */ param->val.nr = 1; @@ -808,24 +819,13 @@ parse_values_std(const SelectionParserValueList &values, SelectionParserValueList::const_iterator value; for (value = values.begin(); value != values.end() && i < param->val.nr; ++value) { - if (value->type != param->val.type) - { - _gmx_selparser_error(scanner, "incorrect value skipped"); - continue; - } + GMX_RELEASE_ASSERT(value->type == param->val.type, + "Invalid value type (should have been caught earlier)"); if (value->hasExpressionValue()) { SelectionTreeElementPointer child = add_child(root, param, value->expr, scanner); - /* Check that the expression is valid */ - if (!child) - { - return false; - } - if (!set_expr_value_store(child, param, i, scanner)) - { - return false; - } + set_expr_value_store(child, param, i, scanner); if (child->flags & SEL_DYNAMIC) { bDynamic = true; @@ -864,8 +864,11 @@ parse_values_std(const SelectionParserValueList &values, case REAL_VALUE: if (value->u.r.r1 != value->u.r.r2) { - _gmx_selparser_error(scanner, "real ranges not supported"); - return false; + std::string text(_gmx_sel_lexer_get_text(scanner, value->location())); + std::string message + = formatString("Real range ('%s') is not supported in this context", + text.c_str()); + GMX_THROW(InvalidInputError(message)); } param->val.u.r[i] = value->u.r.r1; break; @@ -877,20 +880,24 @@ parse_values_std(const SelectionParserValueList &values, break; case NO_VALUE: case GROUP_VALUE: - GMX_THROW(gmx::InternalError("Invalid non-expression value type")); + GMX_THROW(InternalError("Invalid non-expression value type")); } } ++i; } if (value != values.end()) { - _gmx_selparser_error(scanner, "extra values"); - return false; + std::string message + = formatString("Too many values provided, expected %d", + param->val.nr); + GMX_THROW(InvalidInputError(message)); } if (i < param->val.nr) { - _gmx_selparser_error(scanner, "not enough values"); - return false; + std::string message + = formatString("Too few values provided, expected %d", + param->val.nr); + GMX_THROW(InvalidInputError(message)); } if (!bDynamic) { @@ -901,8 +908,6 @@ parse_values_std(const SelectionParserValueList &values, *param->nvalptr = param->val.nr; } param->nvalptr = NULL; - - return true; } /*! \brief @@ -912,19 +917,21 @@ parse_values_std(const SelectionParserValueList &values, * \param[in] values List of values. * \param param Parameter to parse. * \param[in] scanner Scanner data structure. - * \returns true if the values were parsed successfully, false otherwise. */ -static bool +static void parse_values_bool(const std::string &name, const SelectionParserValueList &values, gmx_ana_selparam_t *param, void *scanner) { + GMX_UNUSED_VALUE(scanner); GMX_ASSERT(param->val.type == NO_VALUE, "Boolean parser called for non-boolean parameter"); if (values.size() > 1 || (!values.empty() && values.front().type != INT_VALUE)) { - _gmx_selparser_error(scanner, "parameter takes only a yes/no/on/off/0/1 value"); - return false; + std::string message + = formatString("'%s' only accepts yes/no/on/off/0/1 (and empty) as a value", + param->name); + GMX_THROW(InvalidInputError(message)); } bool bSetNo = false; @@ -936,9 +943,10 @@ parse_values_bool(const std::string &name, } if (bSetNo && !values.empty()) { - _gmx_selparser_error(scanner, "parameter 'no%s' should not have a value", - param->name); - return false; + std::string message + = formatString("'no%s' cannot be followed by any value", + param->name); + GMX_THROW(InvalidInputError(message)); } if (!values.empty() && values.front().u.i.i1 == 0) { @@ -946,7 +954,6 @@ parse_values_bool(const std::string &name, } *param->val.u.b = bSetNo ? false : true; - return true; } /*! \brief @@ -957,7 +964,7 @@ parse_values_bool(const std::string &name, * \param[in] scanner Scanner data structure. * \returns true if the values were parsed successfully, false otherwise. */ -static bool +static void parse_values_enum(const SelectionParserValueList &values, gmx_ana_selparam_t *param, void *scanner) @@ -966,16 +973,19 @@ parse_values_enum(const SelectionParserValueList &values, "Enum parser called for non-string parameter"); if (values.size() != 1) { - _gmx_selparser_error(scanner, "a single value is required"); - return false; + GMX_THROW(InvalidInputError( + "Only a single string value is supported in this context")); } const SelectionParserValue &value = values.front(); GMX_RELEASE_ASSERT(value.type == param->val.type, "Invalid value type (should have been caught earlier)"); if (value.hasExpressionValue()) { - _gmx_selparser_error(scanner, "expression value for enumerated parameter not supported"); - return false; + std::string text(_gmx_sel_lexer_get_text(scanner, value.location())); + std::string message + = formatString("Expression ('%s') is not supported in this context", + text.c_str()); + GMX_THROW(InvalidInputError(message)); } const std::string &svalue = value.stringValue(); @@ -983,13 +993,14 @@ parse_values_enum(const SelectionParserValueList &values, int match = 0; while (param->val.u.s[i] != NULL) { - if (gmx::startsWith(param->val.u.s[i], svalue)) + if (startsWith(param->val.u.s[i], svalue)) { /* Check if there is a duplicate match */ if (match > 0) { - _gmx_selparser_error(scanner, "ambiguous value"); - return false; + std::string message + = formatString("Value '%s' is ambiguous", svalue.c_str()); + GMX_THROW(InvalidInputError(message)); } match = i; } @@ -997,11 +1008,11 @@ parse_values_enum(const SelectionParserValueList &values, } if (match == 0) { - _gmx_selparser_error(scanner, "invalid value"); - return false; + std::string message + = formatString("Value '%s' is not recognized", svalue.c_str()); + GMX_THROW(InvalidInputError(message)); } param->val.u.s[0] = param->val.u.s[match]; - return true; } /*! \brief @@ -1035,8 +1046,8 @@ convert_const_values(SelectionParserValueList *values) *value = SelectionParserValue::createPosition(expr->v.u.p->x[0], location); break; default: - GMX_THROW(gmx::InternalError( - "Unsupported constant expression value type")); + GMX_RELEASE_ASSERT(false, + "Unsupported constant expression value type"); } } } @@ -1048,7 +1059,6 @@ convert_const_values(SelectionParserValueList *values) * \param params Array of parameters to parse. * \param root Selection element to which child expressions are added. * \param[in] scanner Scanner data structure. - * \returns true if the parameters were parsed successfully, false otherwise. * * Initializes the \p params array based on the parameters in \p pparams. * See the documentation of \c gmx_ana_selparam_t for different options @@ -1057,155 +1067,157 @@ convert_const_values(SelectionParserValueList *values) * The list \p pparams and any associated values are freed after the parameters * have been processed, no matter is there was an error or not. */ -bool +void _gmx_sel_parse_params(const gmx::SelectionParserParameterList &pparams, int nparam, gmx_ana_selparam_t *params, const gmx::SelectionTreeElementPointer &root, void *scanner) { - gmx::MessageStringCollector *errors = _gmx_sel_lexer_error_reporter(scanner); - gmx_ana_selparam_t *oparam; - bool bOk, rc; - int i; - + ExceptionInitializer errors(""); /* Check that the value pointers of SPAR_VARNUM parameters are NULL and * that they are not NULL for other parameters */ - bOk = true; - for (i = 0; i < nparam; ++i) + for (int i = 0; i < nparam; ++i) { - std::string contextStr = gmx::formatString("In parameter '%s'", params[i].name); - gmx::MessageStringContext context(errors, contextStr); - if (params[i].val.type != POS_VALUE && (params[i].flags & (SPAR_VARNUM | SPAR_ATOMVAL))) + if (params[i].val.type != POS_VALUE + && (params[i].flags & (SPAR_VARNUM | SPAR_ATOMVAL))) { - if (params[i].val.u.ptr != NULL) - { - _gmx_selparser_error(scanner, "value pointer is not NULL " - "although it should be for SPAR_VARNUM " - "and SPAR_ATOMVAL parameters"); - } - if ((params[i].flags & SPAR_VARNUM) - && (params[i].flags & SPAR_DYNAMIC) && !params[i].nvalptr) - { - _gmx_selparser_error(scanner, "nvalptr is NULL but both " - "SPAR_VARNUM and SPAR_DYNAMIC are specified"); - bOk = false; - } + GMX_RELEASE_ASSERT(params[i].val.u.ptr == NULL, + "value pointer is not NULL " + "although it should be for SPAR_VARNUM " + "and SPAR_ATOMVAL parameters"); + GMX_RELEASE_ASSERT(!((params[i].flags & SPAR_VARNUM) + && (params[i].flags & SPAR_DYNAMIC)) + || params[i].nvalptr != NULL, + "nvalptr is NULL but both " + "SPAR_VARNUM and SPAR_DYNAMIC are specified"); } else { - if (params[i].val.u.ptr == NULL) - { - _gmx_selparser_error(scanner, "value pointer is NULL"); - bOk = false; - } + GMX_RELEASE_ASSERT(params[i].val.u.ptr != NULL, + "value pointer is NULL"); } } - if (!bOk) - { - return false; - } /* Parse the parameters */ - i = 0; + int nullParamIndex = 0; SelectionParserParameterList::const_iterator pparam; for (pparam = pparams.begin(); pparam != pparams.end(); ++pparam) { - std::string contextStr; - /* Find the parameter and make some checks */ - if (!pparam->name().empty()) - { - contextStr = gmx::formatString("In parameter '%s'", pparam->name().c_str()); - i = -1; - oparam = gmx_ana_selparam_find(pparam->name().c_str(), nparam, params); - } - else if (i >= 0) + try { - contextStr = gmx::formatString("In value %d", i + 1); - oparam = ¶ms[i]; - if (oparam->name != NULL) + // Always assigned afterwards, but cppcheck does not see that. + gmx_ana_selparam_t *oparam = NULL; + /* Find the parameter and make some checks */ + if (!pparam->name().empty()) { - oparam = NULL; - _gmx_selparser_error(scanner, "too many NULL parameters provided"); - bOk = false; - continue; + nullParamIndex = -1; + oparam + = gmx_ana_selparam_find(pparam->name().c_str(), nparam, params); + GMX_RELEASE_ASSERT(oparam != NULL, "Inconsistent selection parameter"); } - ++i; - } - else - { - _gmx_selparser_error(scanner, "all NULL parameters should appear in the beginning of the list"); - bOk = false; - continue; - } - gmx::MessageStringContext context(errors, contextStr); - if (!oparam) - { - _gmx_selparser_error(scanner, "unknown parameter skipped"); - bOk = false; - continue; - } - if (oparam->val.type != NO_VALUE && pparam->values().empty()) - { - _gmx_selparser_error(scanner, "no value provided"); - bOk = false; - continue; - } - if (oparam->flags & SPAR_SET) - { - _gmx_selparser_error(scanner, "parameter set multiple times, extra values skipped"); - bOk = false; - continue; - } - oparam->flags |= SPAR_SET; - /* Process the values for the parameter */ - convert_const_values(pparam->values_.get()); - if (convert_values(pparam->values_.get(), oparam->val.type, scanner) != 0) - { - _gmx_selparser_error(scanner, "invalid value"); - bOk = false; - continue; - } - if (oparam->val.type == NO_VALUE) - { - rc = parse_values_bool(pparam->name(), pparam->values(), oparam, scanner); - } - else if (oparam->flags & SPAR_RANGES) - { - rc = parse_values_range(pparam->values(), oparam, scanner); - } - else if (oparam->flags & SPAR_VARNUM) - { - if (pparam->values().size() == 1 - && pparam->values().front().hasExpressionValue()) + else if (nullParamIndex >= 0) { - rc = parse_values_varnum_expr(pparam->values(), oparam, root, scanner); + oparam = ¶ms[nullParamIndex]; + if (oparam->name != NULL) + { + std::string text(_gmx_sel_lexer_get_text(scanner, pparam->location())); + std::string message + = formatString("Unexpected '%s'", text.c_str()); + GMX_THROW(InvalidInputError(message)); + } + ++nullParamIndex; } else { - rc = parse_values_varnum(pparam->values(), oparam, root, scanner); + GMX_RELEASE_ASSERT(false, "All NULL parameters should appear in " + "the beginning of the list"); + } + if (oparam->flags & SPAR_SET) + { + std::string message + = formatString("'%s' appears multiple times", + pparam->name().c_str()); + GMX_THROW(InvalidInputError(message)); + } + oparam->flags |= SPAR_SET; + if (oparam->val.type != NO_VALUE && pparam->values().empty()) + { + std::string text; + if (pparam->name().empty()) + { + text = root->name(); + } + else + { + text = _gmx_sel_lexer_get_text(scanner, pparam->location()); + } + std::string message + = formatString("'%s' should be followed by a value/expression", + text.c_str()); + GMX_THROW(InvalidInputError(message)); + } + /* Process the values for the parameter */ + convert_const_values(pparam->values_.get()); + convert_values(pparam->values_.get(), oparam->val.type, scanner); + if (oparam->val.type == NO_VALUE) + { + parse_values_bool(pparam->name(), pparam->values(), oparam, scanner); + } + else if (oparam->flags & SPAR_RANGES) + { + parse_values_range(pparam->values(), oparam, scanner); + } + else if (oparam->flags & SPAR_VARNUM) + { + if (pparam->values().size() == 1 + && pparam->values().front().hasExpressionValue()) + { + parse_values_varnum_expr(pparam->values(), oparam, root, scanner); + } + else + { + parse_values_varnum(pparam->values(), oparam, root, scanner); + } + } + else if (oparam->flags & SPAR_ENUMVAL) + { + parse_values_enum(pparam->values(), oparam, scanner); + } + else + { + parse_values_std(pparam->values(), oparam, root, scanner); } } - else if (oparam->flags & SPAR_ENUMVAL) - { - rc = parse_values_enum(pparam->values(), oparam, scanner); - } - else - { - rc = parse_values_std(pparam->values(), oparam, root, scanner); - } - if (!rc) + catch (UserInputError &ex) { - bOk = false; + if (!pparam->name().empty()) + { + std::string text(_gmx_sel_lexer_get_text(scanner, pparam->location())); + ex.prependContext(formatString("In '%s'", text.c_str())); + } + errors.addCurrentExceptionAsNested(); } } /* Check that all required parameters are present */ - for (i = 0; i < nparam; ++i) + for (int i = 0; i < nparam; ++i) { if (!(params[i].flags & SPAR_OPTIONAL) && !(params[i].flags & SPAR_SET)) { - _gmx_selparser_error(scanner, "required parameter '%s' not specified", params[i].name); - bOk = false; + std::string message; + if (params[i].name == NULL) + { + message = formatString("'%s' should be followed by a value/expression", + root->name().c_str()); + } + else + { + message = formatString("'%s' is missing", params[i].name); + } + InvalidInputError ex(message); + errors.addNested(ex); } } - - return bOk; + if (errors.hasNestedExceptions()) + { + GMX_THROW(InvalidInputError(errors)); + } } diff --git a/src/gromacs/selection/parsetree.cpp b/src/gromacs/selection/parsetree.cpp index fc80abcf7f..239b2865b1 100644 --- a/src/gromacs/selection/parsetree.cpp +++ b/src/gromacs/selection/parsetree.cpp @@ -314,7 +314,7 @@ _gmx_selparser_handle_exception(yyscan_t scanner, std::exception *ex) bool _gmx_selparser_handle_error(yyscan_t scanner) { - std::string context(gmx::formatString("In selection '%s'", + std::string context(gmx::formatString("Invalid selection '%s'", _gmx_sel_lexer_pselstr(scanner))); // The only way to prepend context to the exception is to rethrow it. try @@ -336,11 +336,8 @@ _gmx_selparser_handle_error(yyscan_t scanner) ex.prependContext(context); throw; } - // Do legacy processing for non-exception cases. - // This is only reached if there was no active exception. - _gmx_selparser_error(scanner, "Invalid selection '%s'", - _gmx_sel_lexer_pselstr(scanner)); - return _gmx_sel_is_lexer_interactive(scanner); + GMX_RELEASE_ASSERT(false, "All parsing errors should result in a captured exception"); + return false; // Some compilers will not believe that the above never returns. } namespace gmx @@ -594,13 +591,11 @@ _gmx_selelem_set_method(const gmx::SelectionTreeElementPointer &sel, * \param[in,out] pcc Position calculation collection to use. * \param[in,out] sel Selection element to initialize. * \param[in] rpost Reference position type to use (NULL = default). - * \param[in] scanner Scanner data structure. - * \returns 0 on success, a non-zero error code on error. */ static void set_refpos_type(gmx::PositionCalculationCollection *pcc, - const SelectionTreeElementPointer &sel, - const char *rpost, yyscan_t scanner) + const SelectionTreeElementPointer &sel, + const char *rpost) { if (!rpost) { @@ -615,9 +610,10 @@ set_refpos_type(gmx::PositionCalculationCollection *pcc, } else { - // TODO: Should this be treated as a real error? - _gmx_selparser_error(scanner, "modifier '%s' is not applicable for '%s'", - rpost, sel->u.expr.method->name); + std::string message + = gmx::formatString("Position modifiers ('%s') is not applicable for '%s'", + rpost, sel->u.expr.method->name); + GMX_THROW(gmx::InvalidInputError(message)); } } @@ -686,11 +682,8 @@ _gmx_sel_init_comparison(const gmx::SelectionTreeElementPointer &left, SelectionParserParameter::create( "op", SelectionParserValue::createString(cmpop, location), location)); - if (!_gmx_sel_parse_params(params, sel->u.expr.method->nparams, - sel->u.expr.method->param, sel, scanner)) - { - return SelectionTreeElementPointer(); - } + _gmx_sel_parse_params(params, sel->u.expr.method->nparams, + sel->u.expr.method->param, sel, scanner); return sel; } @@ -759,13 +752,10 @@ init_keyword_internal(gmx_ana_selmethod_t *method, SelectionParserParameter::createFromExpression(NULL, child)); params.push_back( SelectionParserParameter::create(NULL, move(args), location)); - if (!_gmx_sel_parse_params(params, root->u.expr.method->nparams, - root->u.expr.method->param, root, scanner)) - { - return SelectionTreeElementPointer(); - } + _gmx_sel_parse_params(params, root->u.expr.method->nparams, + root->u.expr.method->param, root, scanner); } - set_refpos_type(&sc->pcc, child, rpost, scanner); + set_refpos_type(&sc->pcc, child, rpost); return root; } @@ -856,29 +846,21 @@ _gmx_sel_init_method(gmx_ana_selmethod_t *method, const char *rpost, yyscan_t scanner) { gmx_ana_selcollection_t *sc = _gmx_sel_lexer_selcollection(scanner); - int rc; gmx::MessageStringCollector *errors = _gmx_sel_lexer_error_reporter(scanner); gmx::MessageStringContext context(errors, formatCurrentErrorContext(scanner)); _gmx_sel_finish_method(scanner); /* The "same" keyword needs some custom massaging of the parameters. */ - rc = _gmx_selelem_custom_init_same(&method, params, scanner); - if (rc != 0) - { - return SelectionTreeElementPointer(); - } + _gmx_selelem_custom_init_same(&method, params, scanner); SelectionTreeElementPointer root( new SelectionTreeElement( SEL_EXPRESSION, _gmx_sel_lexer_get_current_location(scanner))); _gmx_selelem_set_method(root, method, scanner); /* Process the parameters */ - if (!_gmx_sel_parse_params(*params, root->u.expr.method->nparams, - root->u.expr.method->param, root, scanner)) - { - return SelectionTreeElementPointer(); - } - set_refpos_type(&sc->pcc, root, rpost, scanner); + _gmx_sel_parse_params(*params, root->u.expr.method->nparams, + root->u.expr.method->param, root, scanner); + set_refpos_type(&sc->pcc, root, rpost); return root; } @@ -925,11 +907,8 @@ _gmx_sel_init_modifier(gmx_ana_selmethod_t *method, root = modifier; } /* Process the parameters */ - if (!_gmx_sel_parse_params(*params, modifier->u.expr.method->nparams, - modifier->u.expr.method->param, modifier, scanner)) - { - return SelectionTreeElementPointer(); - } + _gmx_sel_parse_params(*params, modifier->u.expr.method->nparams, + modifier->u.expr.method->param, modifier, scanner); return root; } @@ -959,11 +938,8 @@ _gmx_sel_init_position(const gmx::SelectionTreeElementPointer &expr, SelectionParserParameterList params; params.push_back(SelectionParserParameter::createFromExpression(NULL, expr)); /* Parse the parameters. */ - if (!_gmx_sel_parse_params(params, root->u.expr.method->nparams, - root->u.expr.method->param, root, scanner)) - { - return SelectionTreeElementPointer(); - } + _gmx_sel_parse_params(params, root->u.expr.method->nparams, + root->u.expr.method->param, root, scanner); return root; } diff --git a/src/gromacs/selection/parsetree.h b/src/gromacs/selection/parsetree.h index 2b018ca7c2..901795e49b 100644 --- a/src/gromacs/selection/parsetree.h +++ b/src/gromacs/selection/parsetree.h @@ -561,7 +561,7 @@ _gmx_sel_parser_should_finish(void *scanner); /* In params.c */ /** Initializes an array of parameters based on input from the selection parser. */ -bool +void _gmx_sel_parse_params(const gmx::SelectionParserParameterList ¶ms, int nparam, struct gmx_ana_selparam_t *param, const gmx::SelectionTreeElementPointer &root, diff --git a/src/gromacs/selection/sm_keywords.cpp b/src/gromacs/selection/sm_keywords.cpp index 07d47d972d..698aa2bbc3 100644 --- a/src/gromacs/selection/sm_keywords.cpp +++ b/src/gromacs/selection/sm_keywords.cpp @@ -219,9 +219,10 @@ class StringKeywordMatchItem { if (!gmx::Regex::isSupported()) { - GMX_THROW(gmx::InvalidInputError(gmx::formatString( - "No regular expression support, " - "cannot match \"%s\"", str))); + std::string message + = gmx::formatString("No regular expression support, " + "cannot match \"%s\"", str); + GMX_THROW(gmx::InvalidInputError(message)); } regex_.reset(new gmx::Regex(str)); } @@ -746,7 +747,7 @@ evaluate_kweval_pos(t_topology *top, t_trxframe *fr, t_pbc *pbc, * \param[in] method Keyword selection method to evaluate. * \param[in] params Parameters to pass to initialization (the child group). * \param[in] scanner Scanner data structure. - * \returns Pointer to the created selection element (NULL on error). + * \returns Pointer to the created selection element. * * Implements _gmx_sel_init_keyword_evaluator() for \ref GROUP_VALUE input * selections. @@ -759,9 +760,9 @@ init_evaluator_group(gmx_ana_selmethod_t *method, if ((method->flags & (SMETH_SINGLEVAL | SMETH_VARNUMVAL)) || method->outinit || method->pupdate) { - std::string message = gmx::formatString( - "Keyword '%s' cannot be evaluated in this context", - method->name); + std::string message + = gmx::formatString("Keyword '%s' cannot be evaluated in this context", + method->name); GMX_THROW(gmx::InvalidInputError(message)); } @@ -793,11 +794,8 @@ init_evaluator_group(gmx_ana_selmethod_t *method, sel->u.expr.method->param[0].val.u.g = &data->g; - if (!_gmx_sel_parse_params(params, sel->u.expr.method->nparams, - sel->u.expr.method->param, sel, scanner)) - { - return gmx::SelectionTreeElementPointer(); - } + _gmx_sel_parse_params(params, sel->u.expr.method->nparams, + sel->u.expr.method->param, sel, scanner); return sel; } @@ -807,7 +805,7 @@ init_evaluator_group(gmx_ana_selmethod_t *method, * \param[in] method Keyword selection method to evaluate. * \param[in] params Parameters to pass to initialization (the child positions). * \param[in] scanner Scanner data structure. - * \returns Pointer to the created selection element (NULL on error). + * \returns Pointer to the created selection element. * * Implements _gmx_sel_init_keyword_evaluator() for \ref POS_VALUE input * selections. @@ -820,9 +818,9 @@ init_evaluator_pos(gmx_ana_selmethod_t *method, if ((method->flags & (SMETH_SINGLEVAL | SMETH_VARNUMVAL)) || method->outinit || method->pupdate == NULL) { - std::string message = gmx::formatString( - "Keyword '%s' cannot be evaluated in this context", - method->name); + std::string message + = gmx::formatString("Keyword '%s' cannot be evaluated in this context", + method->name); GMX_THROW(gmx::InvalidInputError(message)); } @@ -853,11 +851,8 @@ init_evaluator_pos(gmx_ana_selmethod_t *method, sel->u.expr.method->param[0].val.u.p = &data->p; - if (!_gmx_sel_parse_params(params, sel->u.expr.method->nparams, - sel->u.expr.method->param, sel, scanner)) - { - return gmx::SelectionTreeElementPointer(); - } + _gmx_sel_parse_params(params, sel->u.expr.method->nparams, + sel->u.expr.method->param, sel, scanner); return sel; } @@ -884,6 +879,10 @@ _gmx_sel_init_keyword_evaluator(gmx_ana_selmethod_t *method, } else { - GMX_THROW(gmx::InvalidInputError("Invalid expression for keyword evaluation group")); + std::string text(_gmx_sel_lexer_get_text(scanner, child->location())); + std::string message + = gmx::formatString("Expression '%s' cannot be used to evaluate keywords", + text.c_str()); + GMX_THROW(gmx::InvalidInputError(message)); } } diff --git a/src/gromacs/selection/sm_same.cpp b/src/gromacs/selection/sm_same.cpp index 6362cc5b8e..f97978f988 100644 --- a/src/gromacs/selection/sm_same.cpp +++ b/src/gromacs/selection/sm_same.cpp @@ -231,12 +231,11 @@ init_data_same(int /* npar */, gmx_ana_selparam_t *param) * \param[in,out] method The method to initialize. * \param[in,out] params Pointer to the first parameter. * \param[in] scanner Scanner data structure. - * \returns 0 on success, a non-zero error code on error. * - * If \p *method is not a \c same method, this function returns zero + * If \p *method is not a \c same method, this function returns * immediately. */ -int +void _gmx_selelem_custom_init_same(gmx_ana_selmethod_t **method, const gmx::SelectionParserParameterListPointer ¶ms, void *scanner) @@ -245,15 +244,14 @@ _gmx_selelem_custom_init_same(gmx_ana_selmethod_t **me /* Do nothing if this is not a same method. */ if (!*method || (*method)->name != sm_same.name || params->empty()) { - return 0; + return; } const gmx::SelectionParserValueList &kwvalues = params->front().values(); if (kwvalues.size() != 1 || !kwvalues.front().hasExpressionValue() || kwvalues.front().expr->type != SEL_EXPRESSION) { - _gmx_selparser_error(scanner, "'same' should be followed by a single keyword"); - return -1; + GMX_THROW(gmx::InvalidInputError("'same' should be followed by a single keyword")); } gmx_ana_selmethod_t *kwmethod = kwvalues.front().expr->u.expr.method; if (kwmethod->type == STR_VALUE) @@ -268,8 +266,8 @@ _gmx_selelem_custom_init_same(gmx_ana_selmethod_t **me const gmx::SelectionParserValueList &asvalues = asparam->values(); if (asvalues.size() != 1 || !asvalues.front().hasExpressionValue()) { - _gmx_selparser_error(scanner, "'same ... as' should be followed by a single expression"); - return -1; + // TODO: Think about providing more informative context. + GMX_THROW(gmx::InvalidInputError("'same ... as' should be followed by a single expression")); } const gmx::SelectionTreeElementPointer &child = asvalues.front().expr; /* Create a second keyword evaluation element for the keyword given as @@ -277,16 +275,10 @@ _gmx_selelem_custom_init_same(gmx_ana_selmethod_t **me * second parameter. */ gmx::SelectionTreeElementPointer kwelem = _gmx_sel_init_keyword_evaluator(kwmethod, child, scanner); - // FIXME: Use exceptions. - if (!kwelem) - { - return -1; - } /* Replace the second parameter with one with a value from \p kwelem. */ std::string pname = asparam->name(); *asparam = gmx::SelectionParserParameter::createFromExpression(pname, kwelem); } - return 0; } static void diff --git a/src/gromacs/utility/exceptions.cpp b/src/gromacs/utility/exceptions.cpp index 952383ffad..14fe155e43 100644 --- a/src/gromacs/utility/exceptions.cpp +++ b/src/gromacs/utility/exceptions.cpp @@ -381,6 +381,7 @@ void formatExceptionMessageInternal(MessageWriterInterface *writer, // funcPtr != NULL ? *funcPtr : ""); // } + bool bAnythingWritten = false; // TODO: Remove duplicate context if present in multiple nested exceptions. const ErrorMessage *msg = boost::get_error_info(*boostEx); if (msg != NULL) @@ -394,11 +395,13 @@ void formatExceptionMessageInternal(MessageWriterInterface *writer, if (msg != NULL && !msg->text().empty()) { writer->writeLine(msg->text().c_str(), indent*2); + bAnythingWritten = true; } } else { writer->writeLine(ex.what(), indent*2); + bAnythingWritten = true; } const int *errorNumber @@ -410,6 +413,7 @@ void formatExceptionMessageInternal(MessageWriterInterface *writer, writer->writeErrNoInfo(*errorNumber, funcName != NULL ? *funcName : NULL, (indent+1)*2); + bAnythingWritten = true; } // TODO: Treat also boost::nested_exception (not currently used, though) @@ -427,7 +431,8 @@ void formatExceptionMessageInternal(MessageWriterInterface *writer, } catch (const std::exception &nestedEx) { - formatExceptionMessageInternal(writer, nestedEx, indent + 1); + const int newIndent = indent + (bAnythingWritten ? 1 : 0); + formatExceptionMessageInternal(writer, nestedEx, newIndent); } } } diff --git a/src/gromacs/utility/scoped_cptr.h b/src/gromacs/utility/scoped_cptr.h index f04de31087..1e7909ed31 100644 --- a/src/gromacs/utility/scoped_cptr.h +++ b/src/gromacs/utility/scoped_cptr.h @@ -63,6 +63,9 @@ inline void sfree_wrapper(T *p) * * Currently only implements some operations; other operations can be added * if they become necessary. + * The presence of a release() method is not strictly according to `scoped_ptr` + * design, but makes it easier to make existing C code exception-safe, and does + * not really warrant a separate class for such a purpose. * * This class provides a basic guard/smart pointer for C pointers. * @@ -80,16 +83,20 @@ class scoped_cptr * * \param[in] ptr Pointer to use for initialization. */ - explicit scoped_cptr(T *ptr) : ptr_(ptr) {} + explicit scoped_cptr(T *ptr = NULL) : ptr_(ptr) {} //! Frees the pointer passed to the constructor. ~scoped_cptr() { D(ptr_); } //! Returns the stored pointer. - T * get() const { return ptr_; } + T *get() const { return ptr_; } //! Check for non-null pointer in boolean context. #ifdef GMX_CXX11 explicit #endif operator bool () const { return ptr_ != 0; } + //! Sets the pointer and frees previous pointer if necessary. + void reset(T *ptr) { D(ptr_); ptr_ = ptr; } + //! Clears the pointer without freeing the memory, and returns the old value. + T *release() { T *ptr = ptr_; ptr_ = NULL; return ptr; } private: T *ptr_; -- 2.22.0