Fix another bug in selection subexpression handling.

In this case, it did not work correctly if a variable with a dynamic
value was used both
1. in a boolean expression such that its evaluation group was not known
   in advance, and
2. in a context where it was always evaluated for all atoms (either as
   a selection, or as a parameter to a selection method),
and additionally the latter context appeared later in the selection set.
The initialization of the SEL_CDATA_STATICEVAL flag has a dependency on
SEL_CDATA_FULLEVAL flag, and they were not initialized in the correct
sequence in the above case.

Fixes #1219.

Change-Id: Ic95d37a424c82c796806b3a5b39678e206c467a1

diff --git a/src/gromacs/selection/compiler.cpp b/src/gromacs/selection/compiler.cpp
index 76e083972ba1d2c88415df5542ebece2bcca548f..2b198db6de5b6322d555682f98ec9e1d916b5b67 100644 (file)
--- a/src/gromacs/selection/compiler.cpp
+++ b/src/gromacs/selection/compiler.cpp
@@ -2715,6 +2715,10 @@ SelectionCompiler::compile(SelectionCollection *coll)
     /* Initialize the evaluation callbacks and process the tree structure
      * to conform to the expectations of the callback functions. */
     /* Also, initialize and allocate the compiler data structure */
+    // TODO: Processing the tree in reverse root order would be better,
+    // as it would make dependency handling easier (all subexpression
+    // references would be processed before the actual subexpression) and
+    // could remove the need for most of these extra loops.
     item = sc->root;
     while (item)
     {
@@ -2724,6 +2728,13 @@ SelectionCompiler::compile(SelectionCollection *coll)
         optimize_arithmetic_expressions(item);
         /* Initialize the compiler data */
         init_item_compilerdata(item);
+        item = item->next;
+    }
+    // Initialize the static evaluation compiler flags.
+    // Requires the FULLEVAL compiler flag for the whole tree.
+    item = sc->root;
+    while (item)
+    {
         init_item_staticeval(item);
         init_item_subexpr_refcount(item);
         item = item->next;

diff --git a/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesVariablesWithMixedEvaluationGroups2.xml b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesVariablesWithMixedEvaluationGroups2.xml
new file mode 100644 (file)
index 0000000..46be47d
--- /dev/null
+++ b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesVariablesWithMixedEvaluationGroups2.xml
@@ -0,0 +1,71 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/xsl" href="referencedata.xsl"?>
+<ReferenceData>
+  <ParsedSelections Name="Parsed">
+    <ParsedVariable Name="Variable1">
+      <String Name="Input">foo = atomnr 1 to 8 and x &lt; 10</String>
+    </ParsedVariable>
+    <ParsedSelection Name="Selection1">
+      <String Name="Input">atomnr 1 to 5 and y &lt; 10 and foo</String>
+      <String Name="Name">atomnr 1 to 5 and y &lt; 10 and foo</String>
+      <String Name="Text">atomnr 1 to 5 and y &lt; 10 and foo</String>
+      <Bool Name="Dynamic">true</Bool>
+    </ParsedSelection>
+    <ParsedSelection Name="Selection2">
+      <String Name="Input">foo</String>
+      <String Name="Name">foo</String>
+      <String Name="Text">foo</String>
+      <Bool Name="Dynamic">true</Bool>
+    </ParsedSelection>
+  </ParsedSelections>
+  <CompiledSelections Name="Compiled">
+    <Selection Name="Selection1">
+      <Sequence Name="Atoms">
+        <Int Name="Length">5</Int>
+        <Int>0</Int>
+        <Int>1</Int>
+        <Int>2</Int>
+        <Int>3</Int>
+        <Int>4</Int>
+      </Sequence>
+    </Selection>
+    <Selection Name="Selection2">
+      <Sequence Name="Atoms">
+        <Int Name="Length">8</Int>
+        <Int>0</Int>
+        <Int>1</Int>
+        <Int>2</Int>
+        <Int>3</Int>
+        <Int>4</Int>
+        <Int>5</Int>
+        <Int>6</Int>
+        <Int>7</Int>
+      </Sequence>
+    </Selection>
+  </CompiledSelections>
+  <EvaluatedSelections Name="Frame1">
+    <Selection Name="Selection1">
+      <Sequence Name="Atoms">
+        <Int Name="Length">5</Int>
+        <Int>0</Int>
+        <Int>1</Int>
+        <Int>2</Int>
+        <Int>3</Int>
+        <Int>4</Int>
+      </Sequence>
+    </Selection>
+    <Selection Name="Selection2">
+      <Sequence Name="Atoms">
+        <Int Name="Length">8</Int>
+        <Int>0</Int>
+        <Int>1</Int>
+        <Int>2</Int>
+        <Int>3</Int>
+        <Int>4</Int>
+        <Int>5</Int>
+        <Int>6</Int>
+        <Int>7</Int>
+      </Sequence>
+    </Selection>
+  </EvaluatedSelections>
+</ReferenceData>

diff --git a/src/gromacs/selection/tests/selectioncollection.cpp b/src/gromacs/selection/tests/selectioncollection.cpp
index 491cfbb22dbf18f6a228f238168e33640c4c135e..1038ce309711acf6d448b990d0d582ec0afe4973 100644 (file)
--- a/src/gromacs/selection/tests/selectioncollection.cpp
+++ b/src/gromacs/selection/tests/selectioncollection.cpp
@@ -1108,4 +1108,16 @@ TEST_F(SelectionCollectionDataTest, HandlesVariablesWithMixedEvaluationGroups)
 }
 
 
+TEST_F(SelectionCollectionDataTest, HandlesVariablesWithMixedEvaluationGroups2)
+{
+    static const char * const selections[] = {
+        "foo = atomnr 1 to 8 and x < 10",
+        "atomnr 1 to 5 and y < 10 and foo",
+        "foo"
+    };
+    setFlags(TestFlags() | efTestEvaluation);
+    runTest("simple.gro", selections);
+}
+
+
 } // namespace