zorba-coders team mailing list archive

[Markos Zaharioudakis <markos_za@xxxxxxxxx>]

Markos Zaharioudakis has proposed merging lp:~zorba-coders/zorba/markos-scratch into lp:zorba.

Requested reviews:
  Markos Zaharioudakis (markos-za)

For more details, see:
https://code.launchpad.net/~zorba-coders/zorba/markos-scratch/+merge/103263

Fixed bug #981405
-- 
https://code.launchpad.net/~zorba-coders/zorba/markos-scratch/+merge/103263
Your team Zorba Coders is subscribed to branch lp:zorba.

=== modified file 'ChangeLog'
--- ChangeLog	2012-04-23 23:05:17 +0000
+++ ChangeLog	2012-04-24 12:07:24 +0000
@@ -32,8 +32,13 @@
   * Fixed bug #906494 (default compile with D_FILE_OFFSET_BITS=64)
   * Fixed bug #912586, #912593 and #912722 (assertion failures with lax validation)
   * Fixed bug #921458 (file:read-text-lines() blocking)
+<<<<<<< TREE
   * Fixed bug #947627 (throw XQST0099 if more than one declarations of context item
 	               type in same module)
+=======
+  * Fixed bug #981405 (do not hoist expr containing try-catch variables out of the
+          associated try-catch expression)
+>>>>>>> MERGE-SOURCE
   * Fixed bug #980526 (no-copy rule bug due to global var being set in "distant" udf)
   * Fixed bug #949910 (has-children may be invoked on all nodes). Internally, zorba::store::Item::getChildren() now returns NULL on node classes without offspring (instead of raising an error).
   * Fixed Bug #933490: Error ItemFactoryImpl::createBase64Binary with istream

=== modified file 'src/compiler/expression/expr.h'
--- src/compiler/expression/expr.h	2012-04-23 10:46:38 +0000
+++ src/compiler/expression/expr.h	2012-04-24 12:07:24 +0000
@@ -1010,7 +1010,7 @@
   
   csize clause_count() const { return theCatchClauses.size(); }
   
-  catch_clause_t const& operator[](csize i) const { return theCatchClauses[i]; }
+  const catch_clause_t& operator[](csize i) const { return theCatchClauses[i]; }
 
   void compute_scripting_kind();
 

=== modified file 'src/compiler/rewriter/rules/hoist_rules.cpp'
--- src/compiler/rewriter/rules/hoist_rules.cpp	2012-04-23 23:05:17 +0000
+++ src/compiler/rewriter/rules/hoist_rules.cpp	2012-04-24 12:07:24 +0000
@@ -43,14 +43,14 @@
     expr*,
     const VarIdMap&,
     const ExprVarsMap&,
-    struct flwor_holder*);
+    struct PathHolder*);
 
 static expr_t try_hoisting(
     RewriterContext&,
     expr*,
     const VarIdMap&,
     const ExprVarsMap&,
-    struct flwor_holder*);
+    struct PathHolder*);
 
 static bool non_hoistable (const expr*);
 
@@ -67,15 +67,17 @@
 
 
 /*******************************************************************************
-  Used to implement a stack of flwor exprs.
+  Used to implement a stack of "intersting" exprs that appear in the path from
+  the root expr to the current expr. Currently, "intersting" exprs are flwor
+  and try-catch exprs.
 ********************************************************************************/
-struct flwor_holder
+struct PathHolder
 {
-  struct flwor_holder  * prev;
-  rchandle<flwor_expr>   flwor;
-  long                   clauseCount;
+  struct PathHolder  * prev;
+  expr_t               expr;
+  long                 clauseCount;
 
-  flwor_holder() 
+  PathHolder() 
     :
     prev(NULL),
     clauseCount(0)
@@ -104,13 +106,15 @@
   DynamicBitset freeset(numVars);
   expr_tools::build_expr_to_vars_map(node, varmap, freeset, freevarMap);
 
-  struct flwor_holder root;
+  PathHolder root;
   modified = hoist_expressions(rCtx, node, varmap, freevarMap, &root);
 
-  if (modified && root.flwor != NULL)
+  if (modified && root.expr != NULL)
   {
-    root.flwor->set_return_expr(node);
-    return root.flwor.getp();
+    assert(root.expr->get_expr_kind() == flwor_expr_kind);
+
+    static_cast<flwor_expr*>(root.expr.getp())->set_return_expr(node);
+    return root.expr;
   }
 
   return node;
@@ -125,7 +129,7 @@
     expr* e,
     const VarIdMap& varmap,
     const ExprVarsMap& freevarMap,
-    struct flwor_holder* fholder)
+    struct PathHolder* path)
 {
   bool status = false;
 
@@ -133,12 +137,12 @@
   {
     flwor_expr* flwor = static_cast<flwor_expr *>(e);
 
-    struct flwor_holder curr_holder;
-    curr_holder.prev = fholder;
-    curr_holder.flwor = &*flwor;
+    PathHolder curr_holder;
+    curr_holder.prev = path;
+    curr_holder.expr = e;
 
-    ulong numForLetClauses = flwor->num_forlet_clauses();
-    ulong i = 0;
+    csize numForLetClauses = flwor->num_forlet_clauses();
+    csize i = 0;
 
     while (i < numForLetClauses)
     {
@@ -163,7 +167,7 @@
       }
       else if (domainExpr->is_sequential())
       {
-        struct flwor_holder root;
+        PathHolder root;
         bool hoisted = hoist_expressions(rCtx,
                                          domainExpr,
                                          varmap,
@@ -171,10 +175,13 @@
                                          &root);
         if (hoisted)
         {
-          if (root.flwor != NULL)
+          if (root.expr != NULL)
           {
-            root.flwor->set_return_expr(domainExpr);
-            flc->set_expr(root.flwor.getp());
+            assert(root.expr->get_expr_kind() == flwor_expr_kind);
+
+            static_cast<flwor_expr*>(root.expr.getp())->set_return_expr(domainExpr);
+
+            flc->set_expr(root.expr.getp());
           }
 
           status = true;
@@ -229,13 +236,15 @@
     }
     else if (re->is_sequential())
     {
-      struct flwor_holder root;
+      PathHolder root;
       bool nestedModified = hoist_expressions(rCtx, re, varmap, freevarMap, &root);
 
-      if (nestedModified && root.flwor != NULL)
+      if (nestedModified && root.expr != NULL)
       {
-        root.flwor->set_return_expr(re);
-        flwor->set_return_expr(root.flwor.getp());
+        assert(root.expr->get_expr_kind() == flwor_expr_kind);
+
+        static_cast<flwor_expr*>(root.expr.getp())->set_return_expr(re);
+        flwor->set_return_expr(root.expr.getp());
       }
 
       status = nestedModified || status;
@@ -246,6 +255,35 @@
     }
   }
 
+  else if (e->get_expr_kind() == trycatch_expr_kind)
+  {
+    PathHolder pathStep;
+    pathStep.prev = path;
+    pathStep.expr = e;
+
+    ExprIterator iter(e);
+
+    while(!iter.done())
+    {
+      expr* ce = &*(*iter);
+      if (ce)
+      {
+        expr_t unhoistExpr = try_hoisting(rCtx, ce, varmap, freevarMap, &pathStep);
+        if (unhoistExpr != NULL)
+        {
+          *iter = unhoistExpr.getp();
+          status = true;
+        }
+        else
+        {
+          status = hoist_expressions(rCtx, ce, varmap, freevarMap, &pathStep) || status;
+        }
+      }
+
+      iter.next();
+    }
+  }
+
   else if (e->get_expr_kind() == block_expr_kind || e->is_sequential())
   {
     ExprIterator iter(e);
@@ -257,13 +295,15 @@
       // long as they don't reference any local vars.
       expr_t ce = *iter;
 
-      struct flwor_holder root;
+      PathHolder root;
       bool nestedModified = hoist_expressions(rCtx, ce, varmap, freevarMap, &root);
 
-      if (nestedModified && root.flwor != NULL)
+      if (nestedModified && root.expr != NULL)
       {
-        root.flwor->set_return_expr(ce);
-        (*iter) = root.flwor;
+        assert(root.expr->get_expr_kind() == flwor_expr_kind);
+
+        static_cast<flwor_expr*>(root.expr.getp())->set_return_expr(ce);
+        (*iter) = root.expr;
       }
 
       status = nestedModified || status;
@@ -278,6 +318,7 @@
   {
     // do nothing
   }
+
   else
   {
     ExprIterator iter(e);
@@ -287,7 +328,7 @@
       expr* ce = &*(*iter);
       if (ce)
       {
-        expr_t unhoistExpr = try_hoisting(rCtx, ce, varmap, freevarMap, fholder);
+        expr_t unhoistExpr = try_hoisting(rCtx, ce, varmap, freevarMap, path);
         if (unhoistExpr != NULL)
         {
           *iter = unhoistExpr.getp();
@@ -295,7 +336,7 @@
         }
         else
         {
-          status = hoist_expressions(rCtx, ce, varmap, freevarMap, fholder) || status;
+          status = hoist_expressions(rCtx, ce, varmap, freevarMap, path) || status;
         }
       }
 
@@ -317,7 +358,7 @@
     expr* e,
     const VarIdMap& varmap,
     const ExprVarsMap& freevarMap,
-    struct flwor_holder* holder)
+    struct PathHolder* holder)
 {
   if (non_hoistable(e) || e->contains_node_construction())
   {
@@ -330,7 +371,7 @@
   ZORBA_ASSERT(fvme != freevarMap.end());
   const DynamicBitset& varset = fvme->second;
 
-  struct flwor_holder* h = holder;
+  PathHolder* h = holder;
 
   bool inloop = false;
   bool foundReferencedFLWORVar = false;
@@ -341,62 +382,92 @@
   // result, there is nothing to hoist. 
   while (h->prev != NULL)
   {
-    group_clause* gc = h->flwor->get_group_clause();
-
-    // If any free variable is a group-by variable, give up.
-    if (gc != NULL)
-    {
-      const flwor_clause::rebind_list_t& gvars = gc->get_grouping_vars();
-      ulong numGroupVars = (ulong)gvars.size();
-
-      for (ulong i = 0; i < numGroupVars; ++i)
-      {
-        if (contains_var(gvars[i].second.getp(), varmap, varset))
-          return NULL;
-      }
-
-      const flwor_clause::rebind_list_t& ngvars = gc->get_nongrouping_vars();
-      ulong numNonGroupVars = (ulong)ngvars.size();
-
-      for (ulong i = 0; i < numNonGroupVars; ++i)
-      {
-        if (contains_var(ngvars[i].second.getp(), varmap, varset))
-          return NULL;
-      }
-    }
-
-    // Check whether expr e references any variables from the current flwor. If
-    // not, then e can be hoisted out of the current flwor and we repeat the 
-    // while-loop to see if e can be hoisted w.r.t. the previous (outer) flwor.
-    // If yes, then let V be the inner-most var referenced by e. If there are any
-    // FOR vars after V, e can be hoisted out of any such FOR vars. Otherwise, e
-    // cannot be hoisted.
-    for (i = h->clauseCount - 1; i >= 0; --i)
-    {
-      const forletwin_clause* flc = 
-      static_cast<const forletwin_clause*>(h->flwor->get_clause(i));
-
-      if (flc->get_expr()->is_sequential())
-      {
-        foundSequentialClause = true;
-        break;
-      }
-
-      if (contains_var(flc->get_var(), varmap, varset) ||
-          contains_var(flc->get_pos_var(), varmap, varset) ||
-          contains_var(flc->get_score_var(), varmap, varset))
-      {
-        foundReferencedFLWORVar = true;
-        break;
-      }
-
-      inloop = (inloop ||
-                (flc->get_kind() == flwor_clause::for_clause &&
-                 TypeOps::type_max_cnt(tm, *flc->get_expr()->get_return_type()) >= 2));
-    }
-
-    if (foundSequentialClause || foundReferencedFLWORVar)
-      break;
+    if (h->expr->get_expr_kind() == trycatch_expr_kind)
+    {
+      // Should not hoist an expr out of a try-catch if it contains any try-catch vars
+      trycatch_expr* trycatch = static_cast<trycatch_expr*>(h->expr.getp());
+      csize numClauses = trycatch->clause_count();
+
+      for (csize i = 0; i < numClauses; ++i)
+      {
+        const catch_clause_t& clause = (*trycatch)[i];
+
+        catch_clause::var_map_t& trycatchVars = clause->get_vars();
+
+        catch_clause::var_map_t::const_iterator ite = trycatchVars.begin();
+        catch_clause::var_map_t::const_iterator end = trycatchVars.end();
+        for (; ite != end; ++ite)
+        {
+          const var_expr_t& trycatchVar = (*ite).second;
+
+          if (contains_var(trycatchVar.getp(), varmap, varset))
+            return NULL;
+        } 
+      }
+    }
+    else
+    {
+      assert(h->expr->get_expr_kind() == flwor_expr_kind);
+
+      flwor_expr* flwor = static_cast<flwor_expr*>(h->expr.getp());
+ 
+      group_clause* gc = flwor->get_group_clause();
+
+      // If any free variable is a group-by variable, give up.
+      if (gc != NULL)
+      {
+        const flwor_clause::rebind_list_t& gvars = gc->get_grouping_vars();
+        csize numGroupVars = gvars.size();
+
+        for (csize i = 0; i < numGroupVars; ++i)
+        {
+          if (contains_var(gvars[i].second.getp(), varmap, varset))
+            return NULL;
+        }
+
+        const flwor_clause::rebind_list_t& ngvars = gc->get_nongrouping_vars();
+        csize numNonGroupVars = ngvars.size();
+        
+        for (csize i = 0; i < numNonGroupVars; ++i)
+        {
+          if (contains_var(ngvars[i].second.getp(), varmap, varset))
+            return NULL;
+        }
+      }
+
+      // Check whether expr e references any variables from the current flwor. If
+      // not, then e can be hoisted out of the current flwor and we repeat the 
+      // while-loop to see if e can be hoisted w.r.t. the previous (outer) flwor.
+      // If yes, then let V be the inner-most var referenced by e. If there are any
+      // FOR vars after V, e can be hoisted out of any such FOR vars. Otherwise, e
+      // cannot be hoisted.
+      for (i = h->clauseCount - 1; i >= 0; --i)
+      {
+        const forletwin_clause* flc = 
+        static_cast<const forletwin_clause*>(flwor->get_clause(i));
+
+        if (flc->get_expr()->is_sequential())
+        {
+          foundSequentialClause = true;
+          break;
+        }
+
+        if (contains_var(flc->get_var(), varmap, varset) ||
+            contains_var(flc->get_pos_var(), varmap, varset) ||
+            contains_var(flc->get_score_var(), varmap, varset))
+        {
+          foundReferencedFLWORVar = true;
+          break;
+        }
+        
+        inloop = (inloop ||
+                  (flc->get_kind() == flwor_clause::for_clause &&
+                   TypeOps::type_max_cnt(tm, *flc->get_expr()->get_return_type()) >= 2));
+      }
+
+      if (foundSequentialClause || foundReferencedFLWORVar)
+        break;
+    }
 
     h = h->prev;
   }
@@ -427,15 +498,17 @@
 
   if (h->prev == NULL)
   {
-    if (h->flwor == NULL)
+    if (h->expr == NULL)
     {
-      h->flwor = new flwor_expr(e->get_sctx(), e->get_loc(), false);
+      h->expr = new flwor_expr(e->get_sctx(), e->get_loc(), false);
     }
-    h->flwor->add_clause(flref);
+    static_cast<flwor_expr*>(h->expr.getp())->add_clause(flref);
   }
   else
   {
-    h->flwor->add_clause(i + 1, flref);
+    assert(h->expr->get_expr_kind() == flwor_expr_kind);
+
+    static_cast<flwor_expr*>(h->expr.getp())->add_clause(i + 1, flref);
     ++h->clauseCount;
   }
 

=== modified file 'src/compiler/rewriter/tools/expr_tools.cpp'
--- src/compiler/rewriter/tools/expr_tools.cpp	2012-04-23 10:46:38 +0000
+++ src/compiler/rewriter/tools/expr_tools.cpp	2012-04-24 12:07:24 +0000
@@ -298,6 +298,31 @@
 
     index_flwor_vars(flwor->get_return_expr(), numVars, varidmap, idvarmap);
   }
+  else if (e->get_expr_kind() == trycatch_expr_kind)
+  {
+    const trycatch_expr* trycatch = static_cast<const trycatch_expr*>(e);
+
+    index_flwor_vars(trycatch->get_try_expr(), numVars, varidmap, idvarmap);
+
+    csize numClauses = trycatch->clause_count();
+
+    for (csize i = 0; i < numClauses; ++i)
+    {
+      const catch_clause_t& clause = (*trycatch)[i];
+      
+      catch_clause::var_map_t& trycatchVars = clause->get_vars();
+      
+      catch_clause::var_map_t::const_iterator ite = trycatchVars.begin();
+      catch_clause::var_map_t::const_iterator end = trycatchVars.end();
+      for (; ite != end; ++ite)
+      {
+        const var_expr_t& trycatchVar = (*ite).second;
+        add_var(trycatchVar.getp(), numVars, varidmap, idvarmap);
+      }
+
+      index_flwor_vars(trycatch->get_catch_expr(i), numVars, varidmap, idvarmap);
+    }
+  }
   else
   {
     ExprConstIterator iter(e);
@@ -378,7 +403,7 @@
     return;
   }
 
-  ulong numVars = freeset.size();
+  csize numVars = freeset.size();
 
   DynamicBitset eFreeset(numVars);
   ExprIterator iter(e);
@@ -443,17 +468,17 @@
         const group_clause* gc = static_cast<const group_clause *>(c);
 
         const flwor_clause::rebind_list_t& gvars = gc->get_grouping_vars();
-        unsigned numGroupVars = (unsigned)gvars.size();
+        csize numGroupVars = gvars.size();
 
-        for (unsigned i = 0; i < numGroupVars; ++i)
+        for (csize i = 0; i < numGroupVars; ++i)
         {
           set_bit(gvars[i].second.getp(), varmap, freeset, false);
         }
 
         const flwor_clause::rebind_list_t& ngvars = gc->get_nongrouping_vars();
-        unsigned numNonGroupVars = (unsigned)ngvars.size();
+        csize numNonGroupVars = ngvars.size();
         
-        for (unsigned i = 0; i < numNonGroupVars; ++i)
+        for (csize i = 0; i < numNonGroupVars; ++i)
         {
           set_bit(ngvars[i].second.getp(), varmap, freeset, false);
         }
@@ -466,6 +491,27 @@
       }
     }
   }
+  else if (e->get_expr_kind() == trycatch_expr_kind)
+  {
+    trycatch_expr* trycatch = static_cast<trycatch_expr*>(e);
+
+    csize numClauses = trycatch->clause_count();
+
+    for (csize i = 0; i < numClauses; ++i)
+    {
+      const catch_clause_t& clause = (*trycatch)[i];
+      
+      catch_clause::var_map_t& trycatchVars = clause->get_vars();
+      
+      catch_clause::var_map_t::const_iterator ite = trycatchVars.begin();
+      catch_clause::var_map_t::const_iterator end = trycatchVars.end();
+      for (; ite != end; ++ite)
+      {
+        const var_expr_t& trycatchVar = (*ite).second;
+        set_bit(trycatchVar.getp(), varmap, freeset, false);
+      }
+    }
+  }
 
   freevarMap[e] = freeset;
 }

=== modified file 'src/compiler/translator/translator.cpp'
--- src/compiler/translator/translator.cpp	2012-04-23 23:05:17 +0000
+++ src/compiler/translator/translator.cpp	2012-04-24 12:07:24 +0000
@@ -7238,19 +7238,25 @@
   GENV_ITEMFACTORY->createQName(lStackTrace, ZORBA_ERR_NS, "", "stack-trace");
 
   cc->add_var(catch_clause::err_code,
-      bind_var(loc, lCode.getp(), var_expr::catch_var, theRTM.QNAME_TYPE_ONE) );
+      bind_var(loc, lCode.getp(), var_expr::catch_var, theRTM.QNAME_TYPE_ONE));
+
   cc->add_var(catch_clause::err_desc,
-      bind_var(loc, lDesc.getp(), var_expr::catch_var, theRTM.STRING_TYPE_QUESTION) );
+      bind_var(loc, lDesc.getp(), var_expr::catch_var, theRTM.STRING_TYPE_QUESTION));
+
   cc->add_var(catch_clause::err_value,
-      bind_var(loc, lValue.getp(), var_expr::catch_var, theRTM.ITEM_TYPE_STAR) );
+      bind_var(loc, lValue.getp(), var_expr::catch_var, theRTM.ITEM_TYPE_STAR));
+
   cc->add_var(catch_clause::err_module,
-      bind_var(loc, lModule.getp(), var_expr::catch_var, theRTM.STRING_TYPE_QUESTION) );
+      bind_var(loc, lModule.getp(), var_expr::catch_var, theRTM.STRING_TYPE_QUESTION));
+
   cc->add_var(catch_clause::err_line_no,
-      bind_var(loc, lLineNo.getp(), var_expr::catch_var, theRTM.INTEGER_TYPE_QUESTION) );
+      bind_var(loc, lLineNo.getp(), var_expr::catch_var, theRTM.INTEGER_TYPE_QUESTION));
+
   cc->add_var(catch_clause::err_column_no,
-      bind_var(loc, lColumnNo.getp(), var_expr::catch_var, theRTM.INTEGER_TYPE_QUESTION) );
+      bind_var(loc, lColumnNo.getp(), var_expr::catch_var, theRTM.INTEGER_TYPE_QUESTION));
+
   cc->add_var(catch_clause::zerr_stack_trace,
-      bind_var(loc, lStackTrace.getp(), var_expr::catch_var, theRTM.ITEM_TYPE_QUESTION) );
+      bind_var(loc, lStackTrace.getp(), var_expr::catch_var, theRTM.ITEM_TYPE_QUESTION));
 
   tce->add_clause(cc);
 

=== added file 'test/rbkt/ExpQueryResults/zorba/optim/trycatch_hoist_01.xml.res'
--- test/rbkt/ExpQueryResults/zorba/optim/trycatch_hoist_01.xml.res	1970-01-01 00:00:00 +0000
+++ test/rbkt/ExpQueryResults/zorba/optim/trycatch_hoist_01.xml.res	2012-04-24 12:07:24 +0000
@@ -0,0 +1,2 @@
+<?xml version="1.0" encoding="UTF-8"?>
+1 1 2 3 2 1 2 3 3 1 2 3

=== added file 'test/rbkt/Queries/zorba/optim/trycatch_hoist_01.xq'
--- test/rbkt/Queries/zorba/optim/trycatch_hoist_01.xq	1970-01-01 00:00:00 +0000
+++ test/rbkt/Queries/zorba/optim/trycatch_hoist_01.xq	2012-04-24 12:07:24 +0000
@@ -0,0 +1,17 @@
+
+
+import module namespace reflection = "http://www.zorba-xquery.com/modules/reflection";;
+
+import module namespace err = "http://www.w3.org/2005/xqt-errors";;
+
+for $x in (1,2,3)
+let $code := "($x cast as xs:integer, 1, 2, 3)"
+return
+  try 
+  {
+    reflection:eval($code)
+  }
+  catch err:FORG0001
+  {
+    $err:code eq $err:FORG0001
+  }