Improve diagnostics with empty selections.

Two independent fixes to make empty selections work better:
 - Some empty selections could assert or give internal errors, because
   evaluation of some types of expressions for zero atoms was not
   working.  Fixed by removing the asserts/checks.  The check for empty
   selections should now produce useful diagnostics for these cases.
 - Fix the check for empty selections to work for all types of selection
   option input.  Previously, it only worked if the selection was
   provided on the command line, but not if it was provided from a file
   or through interactive input.

Fixes a problem reported on the gmx-users list.  Only fixed in master
since the problem is only with selections that are nearly always
incorrect.  Also, the empty selections check makes the fix much more
useful there.

Change-Id: Ic3e086e637493fc5fefe4594d4407c5a6c53bc8b

diff --git a/src/gromacs/selection/compiler.cpp b/src/gromacs/selection/compiler.cpp
index 1220c2c0ff73e9271038b3bcba5633cb1a883ef3..98830528888c74a0b18e8821e4c9efe78fac696b 100644 (file)
--- a/src/gromacs/selection/compiler.cpp
+++ b/src/gromacs/selection/compiler.cpp
@@ -1809,8 +1809,6 @@ store_param_val(const SelectionTreeElementPointer &sel)
     if (sel->v.type == INT_VALUE || sel->v.type == REAL_VALUE
         || sel->v.type == STR_VALUE)
     {
-        GMX_RELEASE_ASSERT(sel->v.u.ptr != NULL,
-                           "Selection method parameter not properly initialized");
         _gmx_selvalue_setstore(&sel->u.param->val, sel->v.u.ptr);
     }
 }

diff --git a/src/gromacs/selection/selectionoption.cpp b/src/gromacs/selection/selectionoption.cpp
index 22903f81de8aa80a7688125a86e981f9e0a31753..9685c7c1cb0fd7f9e3229ba3a250b82aa4978bac 100644 (file)
--- a/src/gromacs/selection/selectionoption.cpp
+++ b/src/gromacs/selection/selectionoption.cpp
@@ -108,7 +108,10 @@ void SelectionOptionStorage::addSelections(
         {
             GMX_THROW(InvalidInputError("Dynamic selections not supported"));
         }
-        addValue(*i);
+        // Create a copy to allow modifications.
+        Selection sel(*i);
+        sel.data().setFlags(selectionFlags_);
+        addValue(sel);
     }
     if (bFullValue)
     {
@@ -137,11 +140,6 @@ void SelectionOptionStorage::processSetValues(ValueList *values)
     {
         GMX_THROW(InvalidInputError("Too few (valid) values provided"));
     }
-    ValueList::iterator i;
-    for (i = values->begin(); i != values->end(); ++i)
-    {
-        i->data().setFlags(selectionFlags_);
-    }
 }
 
 void SelectionOptionStorage::processAll()

diff --git a/src/gromacs/selection/sm_compare.cpp b/src/gromacs/selection/sm_compare.cpp
index c34777805e5ce5eac802a321ad8116cb1c576608..1849c1f60dcb69f74bf4a13b09ef8a79acb9a97d 100644 (file)
--- a/src/gromacs/selection/sm_compare.cpp
+++ b/src/gromacs/selection/sm_compare.cpp
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2009,2010,2011,2012, by the GROMACS development team, led by
+ * Copyright (c) 2009,2010,2011,2012,2013, by the GROMACS development team, led by
  * David van der Spoel, Berk Hess, Erik Lindahl, and including many
  * others, as listed in the AUTHORS file in the top-level source
  * directory and at http://www.gromacs.org.
@@ -407,10 +407,6 @@ init_compare(t_topology *top, int npar, gmx_ana_selparam_t *param, void *data)
     /* Store the values */
     n1 = init_comparison_value(&d->left, &param[0]);
     n2 = init_comparison_value(&d->right, &param[3]);
-    if (n1 == 0 || n2 == 0)
-    {
-        GMX_THROW(gmx::InternalError("One of the values for comparison missing"));
-    }
     /* Store the comparison type */
     d->cmpt = comparison_type(d->cmpop);
     if (d->cmpt == CMP_INVALID)

diff --git a/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesEmptySelectionWithUnevaluatedExpressions.xml b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesEmptySelectionWithUnevaluatedExpressions.xml
new file mode 100644 (file)
index 0000000..60208f0
--- /dev/null
+++ b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesEmptySelectionWithUnevaluatedExpressions.xml
@@ -0,0 +1,28 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/xsl" href="referencedata.xsl"?>
+<ReferenceData>
+  <ParsedSelections Name="Parsed">
+    <ParsedSelection Name="Selection1">
+      <String Name="Input">none and x &gt; 2</String>
+      <String Name="Text">none and x &gt; 2</String>
+      <Bool Name="Dynamic">true</Bool>
+    </ParsedSelection>
+    <ParsedSelection Name="Selection2">
+      <String Name="Input">none and same resname as resnr 2</String>
+      <String Name="Text">none and same resname as resnr 2</String>
+      <Bool Name="Dynamic">false</Bool>
+    </ParsedSelection>
+  </ParsedSelections>
+  <CompiledSelections Name="Compiled">
+    <Selection Name="Selection1">
+      <Sequence Name="Atoms">
+        <Int Name="Length">0</Int>
+      </Sequence>
+    </Selection>
+    <Selection Name="Selection2">
+      <Sequence Name="Atoms">
+        <Int Name="Length">0</Int>
+      </Sequence>
+    </Selection>
+  </CompiledSelections>
+</ReferenceData>

diff --git a/src/gromacs/selection/tests/selectioncollection.cpp b/src/gromacs/selection/tests/selectioncollection.cpp
index ffa1b914107179885b2edfb20eb1643c18536ab7..419e62f960d69448ef3483dd6d1b696b92930c67 100644 (file)
--- a/src/gromacs/selection/tests/selectioncollection.cpp
+++ b/src/gromacs/selection/tests/selectioncollection.cpp
@@ -1020,6 +1020,16 @@ TEST_F(SelectionCollectionDataTest, HandlesDynamicAtomValuedParameters)
 }
 
 
+TEST_F(SelectionCollectionDataTest, HandlesEmptySelectionWithUnevaluatedExpressions)
+{
+    static const char * const selections[] = {
+        "none and x > 2",
+        "none and same resname as resnr 2"
+    };
+    runTest("simple.gro", selections);
+}
+
+
 TEST_F(SelectionCollectionDataTest, HandlesNumericComparisons)
 {
     static const char * const selections[] = {

diff --git a/src/gromacs/selection/tests/selectionoption.cpp b/src/gromacs/selection/tests/selectionoption.cpp
index 00af92fac6e35d15785b4af3d364cf64fd1e753d..a64ec9fb4fb9d860832ee84764d3a6af47e25f5c 100644 (file)
--- a/src/gromacs/selection/tests/selectionoption.cpp
+++ b/src/gromacs/selection/tests/selectionoption.cpp
@@ -183,6 +183,26 @@ TEST_F(SelectionOptionTest, ChecksEmptySelections)
 }
 
 
+TEST_F(SelectionOptionTest, ChecksEmptyDelayedSelections)
+{
+    gmx::Selection sel;
+    using gmx::SelectionOption;
+    ASSERT_NO_THROW_GMX(options_.addOption(
+                                SelectionOption("sel").store(&sel)));
+    setManager();
+
+    gmx::OptionsAssigner assigner(&options_);
+    EXPECT_NO_THROW_GMX(assigner.start());
+    ASSERT_NO_THROW_GMX(assigner.startOption("sel"));
+    EXPECT_NO_THROW_GMX(assigner.finishOption());
+    EXPECT_NO_THROW_GMX(assigner.finish());
+    EXPECT_NO_THROW_GMX(options_.finish());
+    ASSERT_NO_THROW_GMX(manager_.parseRequestedFromString("none"));
+
+    EXPECT_THROW_GMX(sc_.compile(), gmx::InvalidInputError);
+}
+
+
 TEST_F(SelectionOptionTest, HandlesTooManySelections)
 {
     gmx::Selection sel;