[Libreoffice-commits] core.git: Branch 'libreoffice-7-0' - basic/source

[Mike Kaganski (via logerrit)]

 basic/source/inc/runtime.hxx     |    3 +-
 basic/source/runtime/runtime.cxx |   58 ++++++++++++++++++++-------------------
 2 files changed, 32 insertions(+), 29 deletions(-)

New commits:
commit 4ef431e0c9306072b011c782b15eed87b7bdfcb5
Author:     Mike Kaganski <mike.kaganski at collabora.com>
AuthorDate: Fri Jul 10 19:06:10 2020 +0300
Commit:     Xisco Fauli <xiscofauli at libreoffice.org>
CommitDate: Tue Jul 14 21:54:11 2020 +0200

    tdf#134576: proper handling of For/For Each with On Error Resume Next
    
    1. In SbiRuntime::PushForEach, always initialize SbiForStack::eForType
    to avoid handling the loop as simple For loop in SbiRuntime::StepNEXT.
    
    2. In SbiRuntime::PushForEach, just like in SbiRuntime::PushFor, don't
    set error, but imitialize the loop properly. This allows to proceed to
    SbiRuntime::StepTESTFOR, where the error will be handled gracefully.
    
    3. In SbiRuntime::StepTESTFOR, check for the proper initialization of
    the iteration, and set the error.
    
    4. In SbiRuntime::StepTESTFOR, on error, modify the type of iteration
    into Error - to make sure that loop ends on next iteration (so only a
    single iteration happens inside the Resume Next).
    
    Change-Id: Idd0bab79fb0099fa3ad295b7fc6ab2eb6173f7b8
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/98537
    Tested-by: Jenkins
    Reviewed-by: Mike Kaganski <mike.kaganski at collabora.com>
    (cherry picked from commit 5760c94b8847164f9a7a181f031c7c86643944af)
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/98758
    Reviewed-by: Xisco Fauli <xiscofauli at libreoffice.org>

diff --git a/basic/source/inc/runtime.hxx b/basic/source/inc/runtime.hxx
index 3b59a5b18341..65be051e704f 100644
--- a/basic/source/inc/runtime.hxx
+++ b/basic/source/inc/runtime.hxx
@@ -55,7 +55,8 @@ enum class ForType {
     To,
     EachArray,
     EachCollection,
-    EachXEnumeration
+    EachXEnumeration,
+    Error,
 };
 
 struct SbiForStack {                // for/next stack:
diff --git a/basic/source/runtime/runtime.cxx b/basic/source/runtime/runtime.cxx
index 71edbd1270ae..774d58cf084a 100644
--- a/basic/source/runtime/runtime.cxx
+++ b/basic/source/runtime/runtime.cxx
@@ -1138,21 +1138,16 @@ void SbiRuntime::PushFor()
 void SbiRuntime::PushForEach()
 {
     SbiForStack* p = new SbiForStack;
+    // Set default value in case of error which is ignored in Resume Next
+    p->eForType = ForType::EachArray;
     p->pNext = pForStk;
     pForStk = p;
 
     SbxVariableRef xObjVar = PopVar();
-    SbxBase* pObj = xObjVar.is() ? xObjVar->GetObject() : nullptr;
-    if( pObj == nullptr )
-    {
-        Error( ERRCODE_BASIC_NO_OBJECT );
-        return;
-    }
+    SbxBase* pObj = xObjVar.is() && xObjVar->IsObject() ? xObjVar->GetObject() : nullptr;
 
-    bool bError_ = false;
     if (SbxDimArray* pArray = dynamic_cast<SbxDimArray*>(pObj))
     {
-        p->eForType = ForType::EachArray;
         p->refEnd = reinterpret_cast<SbxVariable*>(pArray);
 
         sal_Int32 nDims = pArray->GetDims32();
@@ -1196,26 +1191,8 @@ void SbiRuntime::PushForEach()
                 catch(const uno::Exception& )
                 {}
             }
-            if ( !p->xEnumeration.is() )
-            {
-                bError_ = true;
-            }
-        }
-        else
-        {
-            bError_ = true;
         }
     }
-    else
-    {
-        bError_ = true;
-    }
-
-    if( bError_ )
-    {
-        Error( ERRCODE_BASIC_CONVERSION );
-        return;
-    }
 
     // Container variable
     p->refVar = PopVar();
@@ -3045,12 +3022,19 @@ void SbiRuntime::StepTESTFOR( sal_uInt32 nOp1 )
             SbxOperator eOp = ( pForStk->refInc->GetDouble() < 0 ) ? SbxLT : SbxGT;
             if( pForStk->refVar->Compare( eOp, *pForStk->refEnd ) )
                 bEndLoop = true;
+            if (SbxBase::IsError())
+                pForStk->eForType = ForType::Error; // terminate loop at the next iteration
             break;
         }
         case ForType::EachArray:
         {
             SbiForStack* p = pForStk;
-            if( p->pArrayCurIndices == nullptr )
+            if (!p->refEnd)
+            {
+                SbxBase::SetError(ERRCODE_BASIC_CONVERSION);
+                pForStk->eForType = ForType::Error; // terminate loop at the next iteration
+            }
+            else if (p->pArrayCurIndices == nullptr)
             {
                 bEndLoop = true;
             }
@@ -3089,6 +3073,13 @@ void SbiRuntime::StepTESTFOR( sal_uInt32 nOp1 )
         }
         case ForType::EachCollection:
         {
+            if (!pForStk->refEnd)
+            {
+                SbxBase::SetError(ERRCODE_BASIC_CONVERSION);
+                pForStk->eForType = ForType::Error; // terminate loop at the next iteration
+                break;
+            }
+
             BasicCollection* pCollection = static_cast<BasicCollection*>(pForStk->refEnd.get());
             SbxArrayRef xItemArray = pCollection->xItemArray;
             sal_Int32 nCount = xItemArray->Count32();
@@ -3107,7 +3098,12 @@ void SbiRuntime::StepTESTFOR( sal_uInt32 nOp1 )
         case ForType::EachXEnumeration:
         {
             SbiForStack* p = pForStk;
-            if( p->xEnumeration->hasMoreElements() )
+            if (!p->xEnumeration)
+            {
+                SbxBase::SetError(ERRCODE_BASIC_CONVERSION);
+                pForStk->eForType = ForType::Error; // terminate loop at the next iteration
+            }
+            else if (p->xEnumeration->hasMoreElements())
             {
                 Any aElem = p->xEnumeration->nextElement();
                 SbxVariableRef xVar = new SbxVariable( SbxVARIANT );
@@ -3120,6 +3116,12 @@ void SbiRuntime::StepTESTFOR( sal_uInt32 nOp1 )
             }
             break;
         }
+        case ForType::Error:
+        {
+            // We are in Resume Next mode, and we already had one iteration
+            bEndLoop = true;
+            break;
+        }
     }
     if( bEndLoop )
     {