Improve selection error messages (use exceptions)
authorTeemu Murtola <teemu.murtola@gmail.com>
Sat, 8 Nov 2014 10:20:25 +0000 (12:20 +0200)
committerGerrit Code Review <gerrit@gerrit.gromacs.org>
Sat, 24 Jan 2015 15:25:43 +0000 (16:25 +0100)
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
src/gromacs/selection/params.cpp
src/gromacs/selection/parsetree.cpp
src/gromacs/selection/parsetree.h
src/gromacs/selection/sm_keywords.cpp
src/gromacs/selection/sm_same.cpp
src/gromacs/utility/exceptions.cpp
src/gromacs/utility/scoped_cptr.h

index 9401bd629c0fc1ee19de397b451b3ad3e743cada..eb503a7e7e9d4350fc817b200e758a9445edd8c8 100644 (file)
@@ -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 &params,
                               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.
index e1ac68d8289b3224ab9694f4df080b1d82626760..a246a6cb81f2d05a1fbc053c673833bb0d19e2cf 100644 (file)
@@ -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"
 
 #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<int>(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(&param->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 = &param->val.u.p[i]; break;
         case GROUP_VALUE: sel->v.u.g = &param->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(&param->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     = &params[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 = &params[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));
+    }
 }
index fc80abcf7f655b469a8964e83d6207bcf1c93c0e..239b2865b1541266729c10453f99f25abc59c2a9 100644 (file)
@@ -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;
 }
index 2b018ca7c236c8b614c48d7963a6fad331ccdc4e..901795e49bfb718f09907b3301bb1751d372eeea 100644 (file)
@@ -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 &params,
                       int nparam, struct gmx_ana_selparam_t *param,
                       const gmx::SelectionTreeElementPointer &root,
index 07d47d972dca66a4a0e30642f291cefac0cd143b..698aa2bbc312bb15964f664b8ad4214118ffe5f7 100644 (file)
@@ -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));
     }
 }
index 6362cc5b8ec6a235e54deea16ecffa22286a9e23..f97978f9885294cf6dc02e8e401e285a4f16520e 100644 (file)
@@ -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 &params,
                               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
index 952383ffad3c4b86f003be6a2063ed8c422ddb06..14fe155e434e892d378d644b6eecefa24b8e4377 100644 (file)
@@ -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<errinfo_message>(*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);
                 }
             }
         }
index f04de31087c022f893e814610488ca6770646d45..1e7909ed31309803793c699d6582b10d2fa0d014 100644 (file)
@@ -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_;