Rationalize selection element name handling.

The name of SelectionTreeElement is now a plain std::string that manages
its own memory instead of pointing to memory managed somewhere else.
The original design has been a pain to maintain at times, as the memory
for the name easily gets freed, leaving a dangling pointer.

Change-Id: I7b975c67ce9eee7f981128e29d8d9dd75fc70c89

diff --git a/src/gromacs/selection/compiler.cpp b/src/gromacs/selection/compiler.cpp
index a54a173b201ba9a9b287a2d06c1872e4d75f5216..13a3953da3c588ffa80eaa4c2115a5c255ffe7d9 100644 (file)
--- a/src/gromacs/selection/compiler.cpp
+++ b/src/gromacs/selection/compiler.cpp
@@ -688,17 +688,17 @@ remove_unused_subexpressions(SelectionTreeElementPointer root)
  * \param[in]     i   Running number for the subexpression.
  *
  * The name of the selection becomes "SubExpr N", where N is \p i;
- * Memory is allocated for the name and the name is stored both in
- * gmx::SelectionTreeElement::name and gmx::SelectionTreeElement::u::cgrp::name; the latter
- * is freed by _gmx_selelem_free().
+ * Memory is allocated for the name and the name is stored both as the
+ * name of the subexpression element and as
+ * gmx::SelectionTreeElement::u::cgrp::name; the latter is freed by
+ * _gmx_selelem_free().
  */
 static void
 create_subexpression_name(const SelectionTreeElementPointer &sel, int i)
 {
-    char *name = strdup(gmx::formatString("SubExpr %d", i).c_str());
-
-    sel->name        = name;
-    sel->u.cgrp.name = name;
+    std::string name(gmx::formatString("SubExpr %d", i));
+    sel->setName(name);
+    sel->u.cgrp.name = strdup(name.c_str());
 }
 
 /*! \brief
@@ -736,16 +736,8 @@ extract_item_subselections(const SelectionTreeElementPointer &sel,
         {
             subexpr = subexpr->next;
         }
-        /* The latter check excludes variable references.
-         * It also excludes subexpression elements that have already been
-         * processed, because they are given a name when they are first
-         * encountered.
-         * TODO: There should be a more robust mechanism (probably a dedicated
-         * flag) for detecting parser-generated subexpressions than relying on
-         * a NULL name field. Additional TODO: This mechanism doesn't seem to
-         * be currently used. */
-        if (child->type == SEL_SUBEXPRREF && (child->child->type != SEL_SUBEXPR
-                                              || child->child->name == NULL))
+        /* The latter check excludes variable references. */
+        if (child->type == SEL_SUBEXPRREF && child->child->type != SEL_SUBEXPR)
         {
             /* Create the root element for the subexpression */
             if (!root)
@@ -758,24 +750,21 @@ extract_item_subselections(const SelectionTreeElementPointer &sel,
                 subexpr->next.reset(new SelectionTreeElement(SEL_ROOT));
                 subexpr = subexpr->next;
             }
-            /* Create the subexpression element and/or
+            /* Create the subexpression element and
              * move the actual subexpression under the created element. */
-            if (child->child->type != SEL_SUBEXPR)
-            {
-                subexpr->child.reset(new SelectionTreeElement(SEL_SUBEXPR));
-                _gmx_selelem_set_vtype(subexpr->child, child->v.type);
-                subexpr->child->child = child->child;
-                child->child          = subexpr->child;
-            }
-            else
-            {
-                subexpr->child = child->child;
-            }
+            subexpr->child.reset(new SelectionTreeElement(SEL_SUBEXPR));
+            _gmx_selelem_set_vtype(subexpr->child, child->v.type);
+            subexpr->child->child = child->child;
+            child->child          = subexpr->child;
             create_subexpression_name(subexpr->child, ++*subexprn);
             /* Set the flags for the created elements */
             subexpr->flags          |= (child->flags & SEL_VALFLAGMASK);
             subexpr->child->flags   |= (child->flags & SEL_VALFLAGMASK);
         }
+        if (child->type == SEL_SUBEXPRREF)
+        {
+            child->setName(child->child->name());
+        }
         child = child->next;
     }
 
@@ -1117,7 +1106,6 @@ init_item_evalfunc(const SelectionTreeElementPointer &sel)
             break;
 
         case SEL_SUBEXPRREF:
-            sel->name     = sel->child->name;
             sel->evaluate = ((sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR)
                              ? &_gmx_sel_evaluate_subexprref_simple
                              : &_gmx_sel_evaluate_subexprref);
@@ -1624,7 +1612,6 @@ release_subexpr_memory(const SelectionTreeElementPointer &sel)
         if (subexpr.use_count() == 2)
         {
             release_subexpr_memory(subexpr);
-            subexpr->name = NULL;
             // Free children.
             subexpr->child.reset();
             subexpr->freeValues();
@@ -1679,7 +1666,6 @@ make_static(const SelectionTreeElementPointer &sel)
     /* Free the expression data as it is no longer needed */
     sel->freeExpressionData();
     /* Make the item static */
-    sel->name            = NULL;
     sel->type            = SEL_CONST;
     sel->evaluate        = NULL;
     sel->cdata->evaluate = NULL;

diff --git a/src/gromacs/selection/params.cpp b/src/gromacs/selection/params.cpp
index a533180de2c4cbd0fba6cd9f2f698283f3e31528..877cc096376665443e73706907e620a4555d0c11 100644 (file)
--- a/src/gromacs/selection/params.cpp
+++ b/src/gromacs/selection/params.cpp
@@ -593,7 +593,7 @@ parse_values_varnum(int nval, t_selexpr_value *values,
     {
         SelectionTreeElementPointer child(new SelectionTreeElement(SEL_CONST));
         _gmx_selelem_set_vtype(child, STR_VALUE);
-        child->name = param->name;
+        child->setName(param->name);
         child->flags &= ~SEL_ALLOCVAL;
         child->flags |= SEL_FLAGSSET | SEL_VARNUMVAL | SEL_ALLOCDATA;
         child->v.nr = param->val.nr;

diff --git a/src/gromacs/selection/parser.cpp b/src/gromacs/selection/parser.cpp
index 15ea8009a089cf0e09e66e2c7ecd3573a5083f13..617655360bd9322540fd680d4ff0f56a433688ff 100644 (file)
--- a/src/gromacs/selection/parser.cpp
+++ b/src/gromacs/selection/parser.cpp
@@ -1987,7 +1987,7 @@ yyreduce:
                  SelectionTreeElementPointer p
                         = _gmx_sel_init_position(s, NULL, scanner);
                  if (!p) YYERROR;
-                 set((yyval.sel), _gmx_sel_init_selection(s->name, p, scanner));
+                 set((yyval.sel), _gmx_sel_init_selection(s->name().c_str(), p, scanner));
                  END_ACTION;
              }
     break;
@@ -2005,7 +2005,7 @@ yyreduce:
                  SelectionTreeElementPointer p
                         = _gmx_sel_init_position(s, NULL, scanner);
                  if (!p) YYERROR;
-                 set((yyval.sel), _gmx_sel_init_selection(s->name, p, scanner));
+                 set((yyval.sel), _gmx_sel_init_selection(s->name().c_str(), p, scanner));
                  END_ACTION;
              }
     break;

diff --git a/src/gromacs/selection/parser.y b/src/gromacs/selection/parser.y
index 4a02b015807d5e3d637681b2b1044aa1ad3e4b8f..db8431bfc5eef601e44603d0fe1b50eff49cf181 100644 (file)
--- a/src/gromacs/selection/parser.y
+++ b/src/gromacs/selection/parser.y
@@ -231,7 +231,7 @@ cmd_plain:   /* empty */
                  SelectionTreeElementPointer p
                         = _gmx_sel_init_position(s, NULL, scanner);
                  if (!p) YYERROR;
-                 set($$, _gmx_sel_init_selection(s->name, p, scanner));
+                 set($$, _gmx_sel_init_selection(s->name().c_str(), p, scanner));
                  END_ACTION;
              }
            | string
@@ -244,7 +244,7 @@ cmd_plain:   /* empty */
                  SelectionTreeElementPointer p
                         = _gmx_sel_init_position(s, NULL, scanner);
                  if (!p) YYERROR;
-                 set($$, _gmx_sel_init_selection(s->name, p, scanner));
+                 set($$, _gmx_sel_init_selection(s->name().c_str(), p, scanner));
                  END_ACTION;
              }
            | selection

diff --git a/src/gromacs/selection/parsetree.cpp b/src/gromacs/selection/parsetree.cpp
index 09e09710a7a46c1bb62b82e81c01811d5c9a5345..3168400d257c8d9efdc2f455d0f4da2f1c85bd7e 100644 (file)
--- a/src/gromacs/selection/parsetree.cpp
+++ b/src/gromacs/selection/parsetree.cpp
@@ -566,7 +566,7 @@ _gmx_selelem_set_method(const SelectionTreeElementPointer &sel,
                         yyscan_t scanner)
 {
     _gmx_selelem_set_vtype(sel, method->type);
-    sel->name   = method->name;
+    sel->setName(method->name);
     snew(sel->u.expr.method, 1);
     memcpy(sel->u.expr.method, method, sizeof(gmx_ana_selmethod_t));
     _gmx_selelem_init_method_params(sel, scanner);
@@ -634,8 +634,8 @@ _gmx_sel_init_arithmetic(const SelectionTreeElementPointer &left,
     char               buf[2];
     buf[0] = op;
     buf[1] = 0;
+    sel->setName(buf);
     sel->u.arith.opstr = strdup(buf);
-    sel->name          = sel->u.arith.opstr;
     sel->child         = left;
     sel->child->next   = right;
     return sel;
@@ -944,9 +944,9 @@ _gmx_sel_init_group_by_name(const char *name, yyscan_t scanner)
     {
         SelectionTreeElementPointer sel(new SelectionTreeElement(SEL_GROUPREF));
         _gmx_selelem_set_vtype(sel, GROUP_VALUE);
+        sel->setName(name);
         sel->u.gref.name = strdup(name);
         sel->u.gref.id = -1;
-        sel->name = sel->u.gref.name;
         return sel;
     }
     if (!grps)
@@ -962,7 +962,7 @@ _gmx_sel_init_group_by_name(const char *name, yyscan_t scanner)
         _gmx_selparser_error(scanner, "Cannot match 'group %s'", name);
         return SelectionTreeElementPointer();
     }
-    sel->name = sel->u.cgrp.name;
+    sel->setName(sel->u.cgrp.name);
     return sel;
 }
 
@@ -997,7 +997,7 @@ _gmx_sel_init_group_by_id(int id, yyscan_t scanner)
         _gmx_selparser_error(scanner, "Cannot match 'group %d'", id);
         return SelectionTreeElementPointer();
     }
-    sel->name = sel->u.cgrp.name;
+    sel->setName(sel->u.cgrp.name);
     return sel;
 }
 
@@ -1021,7 +1021,7 @@ _gmx_sel_init_variable_ref(const SelectionTreeElementPointer &sel)
     {
         ref.reset(new SelectionTreeElement(SEL_SUBEXPRREF));
         _gmx_selelem_set_vtype(ref, sel->v.type);
-        ref->name  = sel->name;
+        ref->setName(sel->name());
         ref->child = sel;
     }
     return ref;
@@ -1058,7 +1058,8 @@ _gmx_sel_init_selection(const char *name,
     root->child = sel;
     if (name)
     {
-        root->name = root->u.cgrp.name = strdup(name);
+        root->setName(name);
+        root->u.cgrp.name = strdup(name);
     }
     /* Update the flags */
     _gmx_selelem_update_flags(root, scanner);
@@ -1066,7 +1067,7 @@ _gmx_sel_init_selection(const char *name,
     /* If there is no name provided by the user, check whether the actual
      * selection given was from an external group, and if so, use the name
      * of the external group. */
-    if (!root->name)
+    if (root->name().empty())
     {
         SelectionTreeElementPointer child = root->child;
         while (child->type == SEL_MODIFIER)
@@ -1084,15 +1085,17 @@ _gmx_sel_init_selection(const char *name,
             && child->child->child->type == SEL_CONST
             && child->child->child->v.type == GROUP_VALUE)
         {
-            root->name = root->u.cgrp.name =
-                strdup(child->child->child->u.cgrp.name);
+            const char *grpName = child->child->child->u.cgrp.name;
+            root->setName(grpName);
+            root->u.cgrp.name = strdup(grpName);
         }
     }
     /* If there still is no name, use the selection string */
-    if (!root->name)
+    if (root->name().empty())
     {
-        root->name = root->u.cgrp.name
-            = strdup(_gmx_sel_lexer_pselstr(scanner));
+        const char *selStr = _gmx_sel_lexer_pselstr(scanner);
+        root->setName(selStr);
+        root->u.cgrp.name = strdup(selStr);
     }
 
     /* Print out some information if the parser is interactive */
@@ -1153,15 +1156,12 @@ _gmx_sel_assign_variable(const char *name,
     }
     /* Create the root element */
     root.reset(new SelectionTreeElement(SEL_ROOT));
+    root->setName(name);
+    root->u.cgrp.name = strdup(name);
     /* Create the subexpression element */
     root->child.reset(new SelectionTreeElement(SEL_SUBEXPR));
+    root->child->setName(name);
     _gmx_selelem_set_vtype(root->child, expr->v.type);
-    {
-        char *newName = strdup(name);
-        root->name          = newName;
-        root->u.cgrp.name   = newName;
-        root->child->name   = newName;
-    }
     root->child->child  = expr;
     /* Update flags */
     _gmx_selelem_update_flags(root, scanner);

diff --git a/src/gromacs/selection/selection.cpp b/src/gromacs/selection/selection.cpp
index b41574178511b6d25cd76cedb09687f3d8830a4e..a1755c2d708ce70460c5f216484a76da4f105195 100644 (file)
--- a/src/gromacs/selection/selection.cpp
+++ b/src/gromacs/selection/selection.cpp
@@ -49,7 +49,7 @@ namespace internal
 
 SelectionData::SelectionData(SelectionTreeElement *elem,
                              const char *selstr)
-    : name_(elem->name), selectionText_(selstr),
+    : name_(elem->name()), selectionText_(selstr),
       rootElement_(*elem), coveredFractionType_(CFRAC_NONE),
       coveredFraction_(1.0), averageCoveredFraction_(1.0),
       bDynamic_(false), bDynamicCoveredFraction_(false)

diff --git a/src/gromacs/selection/selectioncollection.cpp b/src/gromacs/selection/selectioncollection.cpp
index d09998094fa66dd2badbf92be72423d54e1cd6fc..d55ef58716ff921ed3a18c60978361a22028e002 100644 (file)
--- a/src/gromacs/selection/selectioncollection.cpp
+++ b/src/gromacs/selection/selectioncollection.cpp
@@ -317,7 +317,7 @@ void SelectionCollection::Impl::resolveExternalGroups(
         if (bOk)
         {
             root->type = SEL_CONST;
-            root->name = root->u.cgrp.name;
+            root->setName(root->u.cgrp.name);
         }
     }
 

diff --git a/src/gromacs/selection/selelem.cpp b/src/gromacs/selection/selelem.cpp
index f5e179fea025e613e40a943b5b6baad15ce255fa..1f40b07ba724d2e157704b5035350c7a053ce661 100644 (file)
--- a/src/gromacs/selection/selelem.cpp
+++ b/src/gromacs/selection/selelem.cpp
@@ -118,7 +118,6 @@ namespace gmx
 
 SelectionTreeElement::SelectionTreeElement(e_selelem_t type)
 {
-    this->name       = NULL;
     this->type       = type;
     this->flags      = (type != SEL_ROOT) ? SEL_ALLOCVAL : 0;
     if (type == SEL_BOOLEAN)
@@ -395,9 +394,9 @@ _gmx_selelem_print_tree(FILE *fp, const gmx::SelectionTreeElement &sel,
 
     fprintf(fp, "%*c %s %s", level*2+1, '*',
             _gmx_selelem_type_str(sel), _gmx_sel_value_type_str(&sel.v));
-    if (sel.name)
+    if (!sel.name().empty())
     {
-        fprintf(fp, " \"%s\"", sel.name);
+        fprintf(fp, " \"%s\"", sel.name().c_str());
     }
     fprintf(fp, " flg=");
     if (sel.flags & SEL_FLAGSSET)

diff --git a/src/gromacs/selection/selelem.h b/src/gromacs/selection/selelem.h
index 3b8c8bbf30714e0366fd0a1e5d74bb509a6a31d4..8c562294d58777792e0f1da26b17da29013a4830 100644 (file)
--- a/src/gromacs/selection/selelem.h
+++ b/src/gromacs/selection/selelem.h
@@ -45,6 +45,8 @@
 #ifndef GMX_SELECTION_SELELEM_H
 #define GMX_SELECTION_SELELEM_H
 
+#include <string>
+
 #include <boost/shared_ptr.hpp>
 
 #include "gromacs/legacyheaders/types/simple.h"
@@ -289,13 +291,18 @@ class SelectionTreeElement
          */
         void mempoolRelease();
 
-        /*! \brief Name of the element.
+        //! Returns the name of the element.
+        const std::string &name() const { return name_; }
+        /*! \brief
+         * Sets the name of the element.
          *
-         * This field is only used for informative purposes.
-         * It is always either NULL or a pointer to a string.
-         * Memory is never allocated for it directly.
+         * \param[in] name  Name to set (can be NULL).
+         * \throws    std::bad_alloc if out of memory.
          */
-        const char                         *name;
+        void setName(const char *name) { name_ = (name != NULL ? name : ""); }
+        //! \copydoc setName(const char *)
+        void setName(const std::string &name) { name_ = name; }
+
         //! Type of the element.
         e_selelem_t                         type;
         /*! \brief
@@ -376,6 +383,13 @@ class SelectionTreeElement
         SelectionTreeElementPointer         next;
 
     private:
+        /*! \brief
+         * Name of the element.
+         *
+         * This field is only used for informative purposes.
+         */
+        std::string                         name_;
+
         GMX_DISALLOW_COPY_AND_ASSIGN(SelectionTreeElement);
 };