basic/qa/basic_coverage/test_With.bas | 60 ++++++++++++++++++++++++++++++++++ basic/source/comp/loops.cxx | 50 +++++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 1 deletion(-)
New commits: commit 46dfd2c75bfc9a56659db00f1a8b3946bc8d8340 Author: Mike Kaganski <[email protected]> AuthorDate: Sun Aug 18 12:09:27 2024 +0500 Commit: Xisco Fauli <[email protected]> CommitDate: Mon Aug 19 22:59:18 2024 +0200 Related: tdf#132064 Use set to clear the With internal variable In commit f3f46b5fe729876d128f63f7ab158954ab6657d7 (tdf#132064: make With statement only evaluate its argument once, 2024-08-17), I made a mistake, clearing the internal variable in the end of SbiParser::With using an Erase call, which does not just clears the variable itself, but destroys the underlying object, which is not what we need. I don't know how to refer to a global Nothing object to assign to the variable; so just create an own internal Nothing, and use it in the assignment. Change-Id: Ic8ce52e0402d8461a9b9e4ee07614c4f0a46a95e Reviewed-on: https://gerrit.libreoffice.org/c/core/+/172006 Reviewed-by: Mike Kaganski <[email protected]> Tested-by: Jenkins Signed-off-by: Xisco Fauli <[email protected]> Reviewed-on: https://gerrit.libreoffice.org/c/core/+/172056 diff --git a/basic/qa/basic_coverage/test_With.bas b/basic/qa/basic_coverage/test_With.bas index 389b89090d9b..b7bf439a01a4 100644 --- a/basic/qa/basic_coverage/test_With.bas +++ b/basic/qa/basic_coverage/test_With.bas @@ -45,6 +45,15 @@ Sub test_with ' Field values returned n = 0 s = , expected n = 5 s = bar TestUtil.AssertEqual(fields, "n = 5 s = bar", "Field values") + ' Make sure that With works with the original object, modifies it, and does not destroy + Dim foo_var As New foo + With foo_var + .n = 6 + .s = "baz" + End With + fields = "n = " & foo_var.n & " s = " & foo_var.s + TestUtil.AssertEqual(fields, "n = 6 s = baz", "Field values of foo_var") + Exit Sub errorHandler: TestUtil.ReportErrorHandler("test_with", Err, Error$, Erl) diff --git a/basic/source/comp/loops.cxx b/basic/source/comp/loops.cxx index 1161241a280d..79011e5e3aa9 100644 --- a/basic/source/comp/loops.cxx +++ b/basic/source/comp/loops.cxx @@ -324,9 +324,20 @@ void SbiParser::With() StmntBlock( ENDWITH ); CloseBlock(); - // Erase {_with_library.module_offset} + // {_with_library.module_offset} = Nothing + // I don't know how to refer to the global Nothing constant here; just create an own + constexpr OUString aNothingName = u"{_with_Nothing}"_ustr; + SbiSymDef* pNothingDef = pPool->Find(aNothingName); + if (!pNothingDef) + { + pNothingDef = new SbiSymDef(aNothingName); + pNothingDef->SetType(SbxOBJECT); + pPool->Add(pNothingDef); + aGen.Gen(SbiOpcode::LOCAL_, pNothingDef->GetId(), pNothingDef->GetType()); + } aWithParent.Gen(); - aGen.Gen(SbiOpcode::ERASE_); + SbiExpression(this, *pNothingDef).Gen(); + aGen.Gen(SbiOpcode::SET_); } // LOOP/NEXT/WEND without construct commit bf87977617ead6221e0a42fea3d3e69c983dd805 Author: Mike Kaganski <[email protected]> AuthorDate: Sat Aug 17 22:10:07 2024 +0500 Commit: Xisco Fauli <[email protected]> CommitDate: Mon Aug 19 22:59:08 2024 +0200 tdf#132064: make With statement only evaluate its argument once This is implemented by creating a unique variable at entry of the block, using it as the "with parent", and clearing afterwards. It uses an invalid name, so can't be used by a user. The generation of the bytecode is done in compatible way; only existing opcodes were used (which was the reason to choose the implementation with a variable), so compiled binaries in password-protected libraries are expected to work with older versions. The functional changes are: 1. The argument of With statement is evaluated immediately at the start of the block; previously, it evaluated first time, when the leading dot operator was used, and could even be never evaluated, if the operator wasn't used. This is consistent with VBA, and has a benefit that the lifetime of the object is guaranteed to cover the whole block (might be useful if other block statements depend on the object, similar to Python's 'with' statement contexts). 2. The primary goal of the change: it is only ever evaluated once, no matter how many times was the dot operator used in the block. Change-Id: I4dbd520538a57e3668ab706e8f45740db5d33393 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/171980 Reviewed-by: Mike Kaganski <[email protected]> Tested-by: Jenkins Signed-off-by: Xisco Fauli <[email protected]> Reviewed-on: https://gerrit.libreoffice.org/c/core/+/172055 diff --git a/basic/qa/basic_coverage/test_With.bas b/basic/qa/basic_coverage/test_With.bas new file mode 100644 index 000000000000..389b89090d9b --- /dev/null +++ b/basic/qa/basic_coverage/test_With.bas @@ -0,0 +1,51 @@ +' +' This file is part of the LibreOffice project. +' +' This Source Code Form is subject to the terms of the Mozilla Public +' License, v. 2.0. If a copy of the MPL was not distributed with this +' file, You can obtain one at http://mozilla.org/MPL/2.0/. +' + +Option Explicit + +Function doUnitTest as String + TestUtil.TestInit + test_with + doUnitTest = TestUtil.GetResult() +End Function + +Sub DEV_TST : MsgBox doUnitTesT : End Sub + +Type foo + n As Integer + s As String +End Type + +Dim get_foo_count As Integer + +Function get_foo As foo + get_foo_count = get_foo_count + 1 + get_foo = New foo +End Function + +Sub test_with + On Error GoTo errorHandler + + Dim fields As String + With get_foo() + .n = 5 + .s = "bar" + fields = "n = " & .n & " s = " & .s + End With + ' get_foo must be called only once; before the fix, it failed with + ' Number of calls to get_foo returned 4, expected 1 + TestUtil.AssertEqual(get_foo_count, 1, "Number of calls to get_foo") + ' Before the fix, each use of . resulted in creation of a new 'foo' object, + ' and previous assignments didn't reflect in the result; it failed with + ' Field values returned n = 0 s = , expected n = 5 s = bar + TestUtil.AssertEqual(fields, "n = 5 s = bar", "Field values") + + Exit Sub +errorHandler: + TestUtil.ReportErrorHandler("test_with", Err, Error$, Erl) +End Sub diff --git a/basic/source/comp/loops.cxx b/basic/source/comp/loops.cxx index 1d7f9ff84dfd..1161241a280d 100644 --- a/basic/source/comp/loops.cxx +++ b/basic/source/comp/loops.cxx @@ -22,6 +22,9 @@ #include <memory> #include <basic/sberrors.hxx> +#include <basic/sbmod.hxx> + +#include <rtl/ustrbuf.hxx> // Single-line IF and Multiline IF @@ -287,9 +290,43 @@ void SbiParser::With() pNode->SetType( SbxOBJECT ); - OpenBlock( NIL, aVar.GetExprNode() ); + // Generate a '{_with_library.module_offset} = aVar.GetExprNode()' + // Use the {_with_library.module_offset} in OpenBlock + // The name of the variable can't be used by user: a name like [{_with_library.module_offset}] + // is valid, but not without the square brackets + + // Create the unique name + OUStringBuffer moduleName(aGen.GetModule().GetName()); + for (auto parent = aGen.GetModule().GetParent(); parent; parent = parent->GetParent()) + moduleName.insert(0, parent->GetName() + "."); + + OUString uniqueName = "{_with_" + moduleName + "_" + OUString::number(aGen.GetOffset()) + "}"; + while (pPool->Find(uniqueName) != nullptr) + { + static sal_Int64 unique_suffix; + uniqueName = "{_with_" + moduleName + "_" + OUString::number(aGen.GetOffset()) + "_" + + OUString::number(unique_suffix++) + "}"; + } + SbiSymDef* pWithParentDef = new SbiSymDef(uniqueName); + pWithParentDef->SetType(SbxOBJECT); + pPool->Add(pWithParentDef); + + // DIM local variable: work with Option Explicit + aGen.Gen(SbiOpcode::LOCAL_, pWithParentDef->GetId(), pWithParentDef->GetType()); + + // Assignment + SbiExpression aWithParent(this, *pWithParentDef); + aWithParent.Gen(); + aVar.Gen(); + aGen.Gen(SbiOpcode::SET_); + + OpenBlock(NIL, aWithParent.GetExprNode()); StmntBlock( ENDWITH ); CloseBlock(); + + // Erase {_with_library.module_offset} + aWithParent.Gen(); + aGen.Gen(SbiOpcode::ERASE_); } // LOOP/NEXT/WEND without construct
