sc/Library_sc.mk | 1 sc/qa/unit/units.cxx | 141 ++++++++++++++++ sc/source/core/units/raustack.cxx | 54 ++++++ sc/source/core/units/raustack.hxx | 70 ++++++++ sc/source/core/units/unitsimpl.cxx | 308 ++++++++++++++++++++++++++++--------- sc/source/core/units/unitsimpl.hxx | 57 ++++++ sc/source/core/units/utunit.hxx | 18 ++ 7 files changed, 575 insertions(+), 74 deletions(-)
New commits: commit eba23893a8671d0d951a9b87d355b124024cee24 Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Fri Apr 10 11:03:33 2015 +0100 Move and rename Range/Unit Stack. This in preparation for implementing a combined Unit and Range iterator. Change-Id: I08d28e175453f65c3696e9d1c6c20c7076d9b164 diff --git a/sc/Library_sc.mk b/sc/Library_sc.mk index 778415c..e22bda9 100644 --- a/sc/Library_sc.mk +++ b/sc/Library_sc.mk @@ -688,6 +688,7 @@ $(call gb_Library_add_exception_objects,sc,\ ifeq ($(ENABLE_CALC_UNITVERIFICATION),TRUE) $(eval $(call gb_Library_add_exception_objects,sc,\ + sc/source/core/units/raustack \ sc/source/core/units/units \ sc/source/core/units/unitsimpl \ sc/source/core/units/util \ diff --git a/sc/source/core/units/raustack.cxx b/sc/source/core/units/raustack.cxx new file mode 100644 index 0000000..f838acf --- /dev/null +++ b/sc/source/core/units/raustack.cxx @@ -0,0 +1,54 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ +/* + * 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/. + * + */ +#include "raustack.hxx" + +using namespace sc::units; + +RangeListIterator::RangeListIterator(ScDocument* pDoc, const ScRangeList& rRangeList) + : + mRangeList(rRangeList), + mpDoc(pDoc), + mIt(pDoc, ScRange()), + nCurrentIndex(0) +{ +} + +bool RangeListIterator::first() { + if (mRangeList.size() > 0) { + mIt = ScCellIterator(mpDoc, *mRangeList[0]); + return mIt.first(); + } else { + return false; + } +} + +const ScAddress& RangeListIterator::GetPos() const { + return mIt.GetPos(); +} + +bool RangeListIterator::next() { + if (!(mRangeList.size() > 0) || nCurrentIndex >= mRangeList.size()) { + return false; + } + + if (mIt.next()) { + return true; + } else if (++nCurrentIndex < mRangeList.size()) { + mIt = ScCellIterator(mpDoc, *mRangeList[nCurrentIndex]); + mIt.first(); + // TODO: if emtpy - skip to next...? + return true; + } else { + return false; + } +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ + diff --git a/sc/source/core/units/raustack.hxx b/sc/source/core/units/raustack.hxx new file mode 100644 index 0000000..b5cf48b --- /dev/null +++ b/sc/source/core/units/raustack.hxx @@ -0,0 +1,70 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ +/* + * 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/. + * + */ +#ifndef INCLUDED_SC_SOURCE_CORE_UNITS_RAUSTACK_HXX +#define INCLUDED_SC_SOURCE_CORE_UNITS_RAUSTACK_HXX + +#include <boost/variant.hpp> + +#include <address.hxx> +#include <dociter.hxx> +#include <rangelst.hxx> + + +#include "utunit.hxx" + +namespace sc { +namespace units { + +enum class RAUSItemType { + UNITS, + RANGE +}; + +struct RAUSItem { + RAUSItemType type; + boost::variant< ScRange, UtUnit > item; +}; + +class RangeListIterator { +private: + const ScRangeList mRangeList; + ScDocument* mpDoc; + + ScCellIterator mIt; + size_t nCurrentIndex; + +public: + RangeListIterator(ScDocument* pDoc, const ScRangeList& rRangeList); + + bool first(); + + const ScAddress& GetPos() const; + + bool next(); +}; + +}} // sc::units + +// class RangeAndUnitStack { + + + +// }; + +// class RATSIterator { +// public: +// // TODO: need to be able to return non-initialisation +// static RATSIterator getIterator(RangeAndTokenStack& rStack, ScDoc* pDoc, int nItems); + +// } + +#endif // INCLUDED_SC_SOURCE_CORE_UNITS_RAUSTACK_HXX + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/sc/source/core/units/unitsimpl.cxx b/sc/source/core/units/unitsimpl.cxx index 99e03af..e8fb261 100644 --- a/sc/source/core/units/unitsimpl.cxx +++ b/sc/source/core/units/unitsimpl.cxx @@ -11,7 +11,6 @@ #include "util.hxx" -#include "dociter.hxx" #include "document.hxx" #include "refdata.hxx" #include "stringutil.hxx" @@ -85,55 +84,7 @@ UnitsImpl::~UnitsImpl() { // (i.e. if udunits can't handle being used across threads)? } -class RangeListIterator { -private: - const ScRangeList mRangeList; - ScDocument* mpDoc; - - ScCellIterator mIt; - size_t nCurrentIndex; - -public: - RangeListIterator(ScDocument* pDoc, const ScRangeList& rRangeList) - : - mRangeList(rRangeList), - mpDoc(pDoc), - mIt(pDoc, ScRange()), - nCurrentIndex(0) - { - } - - bool first() { - if (mRangeList.size() > 0) { - mIt = ScCellIterator(mpDoc, *mRangeList[0]); - return mIt.first(); - } else { - return false; - } - } - - const ScAddress& GetPos() const { - return mIt.GetPos(); - } - - bool next() { - if (!(mRangeList.size() > 0) || nCurrentIndex >= mRangeList.size()) { - return false; - } - - if (mIt.next()) { - return true; - } else if (++nCurrentIndex < mRangeList.size()) { - mIt = ScCellIterator(mpDoc, *mRangeList[nCurrentIndex]); - mIt.first(); - return true; - } else { - return false; - } - } -}; - -UnitsResult UnitsImpl::getOutputUnitsForOpCode(stack< StackItem >& rStack, const formula::FormulaToken* pToken, ScDocument* pDoc) { +UnitsResult UnitsImpl::getOutputUnitsForOpCode(stack< RAUSItem >& rStack, const formula::FormulaToken* pToken, ScDocument* pDoc) { const OpCode aOpCode = pToken->GetOpCode(); auto nOpCode = static_cast<std::underlying_type<const OpCode>::type>(aOpCode); @@ -147,7 +98,7 @@ UnitsResult UnitsImpl::getOutputUnitsForOpCode(stack< StackItem >& rStack, const if (rStack.size() == 0) { SAL_WARN("sc.units", "Single item opcode failed (no stack items, or range used)"); return { UnitsStatus::UNITS_INVALID, boost::none }; - } else if (rStack.top().type != StackItemType::UNITS) { + } else if (rStack.top().type != RAUSItemType::UNITS) { return { UnitsStatus::UNITS_UNKNOWN, boost::none }; } @@ -183,13 +134,13 @@ UnitsResult UnitsImpl::getOutputUnitsForOpCode(stack< StackItem >& rStack, const return { UnitsStatus::UNITS_INVALID, boost::none }; } - if (rStack.top().type != StackItemType::UNITS) { + if (rStack.top().type != RAUSItemType::UNITS) { return { UnitsStatus::UNITS_UNKNOWN, boost::none }; } UtUnit pSecondUnit = boost::get<UtUnit>(rStack.top().item); rStack.pop(); - if (rStack.top().type != StackItemType::UNITS) { + if (rStack.top().type != RAUSItemType::UNITS) { return { UnitsStatus::UNITS_UNKNOWN, boost::none }; } UtUnit pFirstUnit = boost::get<UtUnit>(rStack.top().item); @@ -240,12 +191,12 @@ UnitsResult UnitsImpl::getOutputUnitsForOpCode(stack< StackItem >& rStack, const for ( ; nParams > 0; nParams--) { switch (rStack.top().type) { - case StackItemType::UNITS: + case RAUSItemType::UNITS: { aUnitsStack.push(boost::get< UtUnit >(rStack.top().item)); break; } - case StackItemType::RANGE: + case RAUSItemType::RANGE: { aRangeList.Append(boost::get< ScRange >(rStack.top().item)); break; @@ -545,7 +496,7 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd pArray->Dump(); #endif - stack< StackItem > aStack; + stack< RAUSItem > aStack; for (FormulaToken* pToken = pArray->FirstRPN(); pToken != 0; pToken = pArray->NextRPN()) { switch (pToken->GetType()) { @@ -564,14 +515,14 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd return false; } - aStack.push( { StackItemType::UNITS, aUnit } ); + aStack.push( { RAUSItemType::UNITS, aUnit } ); break; } case formula::svDoubleRef: { ScComplexRefData* pDoubleRef = pToken->GetDoubleRef(); ScRange aRange = pDoubleRef->toAbs(rFormulaAddress); - aStack.push( { StackItemType::RANGE, aRange } ); + aStack.push( { RAUSItemType::RANGE, aRange } ); break; } @@ -588,7 +539,7 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd case UnitsStatus::UNITS_VALID: assert(aResult.units); // ensure that we have the optional unit assert(aResult.units->isValid()); - aStack.push( { StackItemType::UNITS, aResult.units.get() } ); + aStack.push( { RAUSItemType::UNITS, aResult.units.get() } ); break; } @@ -603,7 +554,7 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd UtUnit::createUnit("", aScale, mpUnitSystem); // Get the dimensionless unit 1 aScale = aScale*(pToken->GetDouble()); - aStack.push( { StackItemType::UNITS, aScale } ); + aStack.push( { RAUSItemType::UNITS, aScale } ); break; } @@ -619,7 +570,7 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd if (aStack.size() != 1) { SAL_WARN("sc.units", "Wrong number of units on stack, should be 1, actual number: " << aStack.size()); return false; - } else if (aStack.top().type != StackItemType::UNITS) { + } else if (aStack.top().type != RAUSItemType::UNITS) { SAL_WARN("sc.units", "End of verification: item on stack does not contain units"); return false; } diff --git a/sc/source/core/units/unitsimpl.hxx b/sc/source/core/units/unitsimpl.hxx index 6abffdd..379a6a7 100644 --- a/sc/source/core/units/unitsimpl.hxx +++ b/sc/source/core/units/unitsimpl.hxx @@ -13,7 +13,6 @@ #include <boost/optional.hpp> #include <boost/scoped_ptr.hpp> #include <boost/shared_ptr.hpp> -#include <boost/variant.hpp> #include <boost/weak_ptr.hpp> #include <formula/opcode.hxx> @@ -25,6 +24,7 @@ #include "rangelst.hxx" #include "units.hxx" +#include "raustack.hxx" #include "utunit.hxx" #include <stack> @@ -42,16 +42,6 @@ namespace test { class UnitsTest; } -enum class StackItemType { - UNITS, - RANGE -}; - -struct StackItem { - StackItemType type; - boost::variant< ScRange, UtUnit > item; -}; - enum class UnitsStatus { UNITS_VALID, UNITS_UNKNOWN, @@ -103,7 +93,7 @@ public: const OUString& rsOldUnit) SAL_OVERRIDE; private: - UnitsResult getOutputUnitsForOpCode(std::stack< StackItem >& rStack, const formula::FormulaToken* pToken, ScDocument* pDoc); + UnitsResult getOutputUnitsForOpCode(std::stack< RAUSItem >& rStack, const formula::FormulaToken* pToken, ScDocument* pDoc); /** * Find and extract a Unit in the standard header notation, commit d9bdadb13fa78593c63d561fa00dbbb4c9a72245 Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Wed Apr 1 14:54:20 2015 +0200 Check output unit against header definition during verification. Change-Id: I98a706d80eb442d274fc111fb6c22e43d79fb9ff diff --git a/sc/source/core/units/unitsimpl.cxx b/sc/source/core/units/unitsimpl.cxx index fbd0c2d..99e03af 100644 --- a/sc/source/core/units/unitsimpl.cxx +++ b/sc/source/core/units/unitsimpl.cxx @@ -616,7 +616,23 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd } } - // TODO: only fail if actual parsing fails? + if (aStack.size() != 1) { + SAL_WARN("sc.units", "Wrong number of units on stack, should be 1, actual number: " << aStack.size()); + return false; + } else if (aStack.top().type != StackItemType::UNITS) { + SAL_WARN("sc.units", "End of verification: item on stack does not contain units"); + return false; + } + + OUString sUnitString; + ScAddress aAddress; + + UtUnit aHeaderUnit = findHeaderUnitForCell(rFormulaAddress, pDoc, sUnitString, aAddress); + UtUnit aResultUnit = boost::get< UtUnit>(aStack.top().item); + + if (aHeaderUnit.isValid() && aHeaderUnit != aResultUnit) { + return false; + } return true; } commit f1328fab55923b99b2c08dc5fbba0b4dfff2eda4 Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Wed Apr 1 13:48:01 2015 +0200 Use correct stack for multiparam opcodes. Also add an appropriate unit test. Change-Id: Ie2e9cce74563dea80b33f5e8238ba6ad1aae1b49 diff --git a/sc/qa/unit/units.cxx b/sc/qa/unit/units.cxx index ac1bfa5..7b5fd25 100644 --- a/sc/qa/unit/units.cxx +++ b/sc/qa/unit/units.cxx @@ -291,7 +291,6 @@ void UnitsTest::testUnitVerification() { // SUM("cm"&"kg") - multiple ranges address.IncRow(); - // TODO: by default / when testing: separator is...? mpDoc->SetFormula(address, "=SUM(A1:A3,B1:B3)"); pCell = mpDoc->GetFormulaCell(address); pTokens = pCell->GetCode(); @@ -325,6 +324,23 @@ void UnitsTest::testUnitVerification() { pTokens = pCell->GetCode(); CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + // Test that multiple arguments of mixed types work too + // Adding multiple cells from one column is ok + address.IncRow(); + mpDoc->SetFormula(address, "=SUM(A1,A2:A3)"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // But mixing the columns fails because of mixed units + // (This test is primarily to ensure that we can handle arbitrary numbers + // of arguments.) + address.IncRow(); + mpDoc->SetFormula(address, "=SUM(A1,A2:A3,B1:B3,C1,C2,C3)"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + // Do a quick sanity check for all the other supported functions // The following all expect identically united inputs, and should // preserve that unit: diff --git a/sc/source/core/units/unitsimpl.cxx b/sc/source/core/units/unitsimpl.cxx index d49799f..fbd0c2d 100644 --- a/sc/source/core/units/unitsimpl.cxx +++ b/sc/source/core/units/unitsimpl.cxx @@ -274,7 +274,7 @@ UnitsResult UnitsImpl::getOutputUnitsForOpCode(stack< StackItem >& rStack, const return { UnitsStatus::UNITS_INVALID, boost::none }; } } - rStack.pop(); + aUnitsStack.pop(); } if (aIt.first()) { commit d25ffe2e1d65a06e85b5f7f278a977bbd679742c Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Wed Apr 1 13:21:35 2015 +0200 Refactor and rewrite range and multiparam opcode handling. Change-Id: Ic52bfb7daae44ea8f8af710fa70f1f30150fc274 diff --git a/sc/source/core/units/unitsimpl.cxx b/sc/source/core/units/unitsimpl.cxx index 6057a0e..d49799f 100644 --- a/sc/source/core/units/unitsimpl.cxx +++ b/sc/source/core/units/unitsimpl.cxx @@ -85,26 +85,76 @@ UnitsImpl::~UnitsImpl() { // (i.e. if udunits can't handle being used across threads)? } -UnitsResult UnitsImpl::getOutputUnitsForOpCode(stack< UtUnit >& rUnitStack, const OpCode& rOpCode) { - UtUnit pOut; +class RangeListIterator { +private: + const ScRangeList mRangeList; + ScDocument* mpDoc; - auto nOpCode = static_cast<std::underlying_type<const OpCode>::type>(rOpCode); + ScCellIterator mIt; + size_t nCurrentIndex; + +public: + RangeListIterator(ScDocument* pDoc, const ScRangeList& rRangeList) + : + mRangeList(rRangeList), + mpDoc(pDoc), + mIt(pDoc, ScRange()), + nCurrentIndex(0) + { + } + bool first() { + if (mRangeList.size() > 0) { + mIt = ScCellIterator(mpDoc, *mRangeList[0]); + return mIt.first(); + } else { + return false; + } + } + + const ScAddress& GetPos() const { + return mIt.GetPos(); + } + + bool next() { + if (!(mRangeList.size() > 0) || nCurrentIndex >= mRangeList.size()) { + return false; + } + + if (mIt.next()) { + return true; + } else if (++nCurrentIndex < mRangeList.size()) { + mIt = ScCellIterator(mpDoc, *mRangeList[nCurrentIndex]); + mIt.first(); + return true; + } else { + return false; + } + } +}; + +UnitsResult UnitsImpl::getOutputUnitsForOpCode(stack< StackItem >& rStack, const formula::FormulaToken* pToken, ScDocument* pDoc) { + const OpCode aOpCode = pToken->GetOpCode(); + auto nOpCode = static_cast<std::underlying_type<const OpCode>::type>(aOpCode); + + UtUnit pOut; // TODO: sc/source/core/tool/parclass.cxx has a mapping of opcodes to possible operands, which we // should probably be using in practice. if (nOpCode >= SC_OPCODE_START_UN_OP && nOpCode < SC_OPCODE_STOP_UN_OP) { - if (!(rUnitStack.size() >= 1)) { - SAL_WARN("sc.units", "no units on stack for unary operation"); + if (rStack.size() == 0) { + SAL_WARN("sc.units", "Single item opcode failed (no stack items, or range used)"); return { UnitsStatus::UNITS_INVALID, boost::none }; + } else if (rStack.top().type != StackItemType::UNITS) { + return { UnitsStatus::UNITS_UNKNOWN, boost::none }; } - UtUnit pUnit = rUnitStack.top(); - rUnitStack.pop(); + UtUnit pUnit = boost::get<UtUnit>(rStack.top().item);//rStack.top().item.get< UtUnit >(); + rStack.pop(); - switch (rOpCode) { + switch (aOpCode) { case ocNot: if (!pUnit.isDimensionless()) { return { UnitsStatus::UNITS_INVALID, boost::none }; @@ -128,19 +178,24 @@ UnitsResult UnitsImpl::getOutputUnitsForOpCode(stack< UtUnit >& rUnitStack, cons } else if (nOpCode >= SC_OPCODE_START_BIN_OP && nOpCode < SC_OPCODE_STOP_BIN_OP) { - if (!(rUnitStack.size() >= 2)) { - SAL_WARN("sc.units", "less than two units on stack when attempting binary operation"); - // TODO: what should we be telling the user in this case? Can this even happen (i.e. - // should we just be asserting here?) + if (rStack.size() < 2) { + SAL_WARN("sc.units", "less than two items on stack when attempting binary operation"); return { UnitsStatus::UNITS_INVALID, boost::none }; } - UtUnit pSecondUnit = rUnitStack.top(); - rUnitStack.pop(); - UtUnit pFirstUnit = rUnitStack.top(); - rUnitStack.pop(); + if (rStack.top().type != StackItemType::UNITS) { + return { UnitsStatus::UNITS_UNKNOWN, boost::none }; + } + UtUnit pSecondUnit = boost::get<UtUnit>(rStack.top().item); + rStack.pop(); + + if (rStack.top().type != StackItemType::UNITS) { + return { UnitsStatus::UNITS_UNKNOWN, boost::none }; + } + UtUnit pFirstUnit = boost::get<UtUnit>(rStack.top().item); + rStack.pop(); - switch (rOpCode) { + switch (aOpCode) { case ocAdd: // Adding and subtracting both require the same units on both sides // hence we can just fall through / use the same logic. @@ -164,90 +219,111 @@ UnitsResult UnitsImpl::getOutputUnitsForOpCode(stack< UtUnit >& rUnitStack, cons SAL_INFO("sc.units", "unit verification not supported for opcode: " << nOpCode); assert(false); } + } else if (nOpCode >= SC_OPCODE_START_2_PAR && + nOpCode < SC_OPCODE_STOP_2_PAR) { + sal_uInt8 nParams = pToken->GetByte(); - } else { - SAL_INFO("sc.units", "unit verification not supported for opcode: " << nOpCode); - return { UnitsStatus::UNITS_UNKNOWN, boost::none }; - } - // TODO: else if unary, or no params, or ... - // TODO: implement further sensible opcode handling - return { UnitsStatus::UNITS_VALID, pOut }; -} - -class RangeListIterator { -private: - const ScRangeList mRangeList; - ScDocument* mpDoc; - - ScCellIterator mIt; - size_t nCurrentIndex; + assert(nParams <= rStack.size()); -public: - RangeListIterator(ScDocument* pDoc, const ScRangeList& rRangeList) - : - mRangeList(rRangeList), - mpDoc(pDoc), - mIt(pDoc, ScRange()), - nCurrentIndex(0) - { - if (rRangeList.size() > 0) { - mIt = ScCellIterator(pDoc, *rRangeList[0]); - mIt.first(); + // If there are no input parameters then the output unit is undefined. + // (In practice this would probably be nonsensical, but isn't a unit + // error per-se.) + if (nParams == 0) { + return { UnitsStatus::UNITS_UNKNOWN, boost::none }; } - } - const ScAddress& GetPos() const { - return mIt.GetPos(); - } + // This is still quite an ugly solution, even better would maybe be to have + // a StackUnitIterator which iterates all the appropriate units given a number of stack items + // to iterate over? + ScRangeList aRangeList; + stack< UtUnit > aUnitsStack; + + for ( ; nParams > 0; nParams--) { + switch (rStack.top().type) { + case StackItemType::UNITS: + { + aUnitsStack.push(boost::get< UtUnit >(rStack.top().item)); + break; + } + case StackItemType::RANGE: + { + aRangeList.Append(boost::get< ScRange >(rStack.top().item)); + break; + } + } - bool next() { - if (!(mRangeList.size() > 0) || nCurrentIndex >= mRangeList.size()) { - return false; + rStack.pop(); } - if (mIt.next()) { - return true; - } else if (++nCurrentIndex < mRangeList.size()) { - mIt = ScCellIterator(mpDoc, *mRangeList[nCurrentIndex]); - mIt.first(); - return true; - } else { - return false; - } - } -}; + RangeListIterator aIt(pDoc, aRangeList); -UnitsResult UnitsImpl::getOutputUnitForDoubleRefOpcode(const OpCode& rOpCode, const ScRangeList& rRangeList, ScDocument* pDoc) { - RangeListIterator aIt(pDoc, rRangeList); + switch (aOpCode) { + case ocSum: + case ocMin: + case ocMax: + case ocAverage: + { + boost::optional< UtUnit > aFirstUnit; + + while (aUnitsStack.size() > 0) { + if (!aFirstUnit) { + aFirstUnit = aUnitsStack.top(); + } else { + UtUnit aCurrentUnit(aUnitsStack.top()); + if (aFirstUnit.get() != aCurrentUnit) { + return { UnitsStatus::UNITS_INVALID, boost::none }; + } + } + rStack.pop(); + } - switch (rOpCode) { - case ocSum: - case ocMin: - case ocMax: - case ocAverage: - { - UtUnit aFirstUnit = getUnitForCell(aIt.GetPos(), pDoc); + if (aIt.first()) { + do { + if (!aFirstUnit) { + aFirstUnit = getUnitForCell(aIt.GetPos(), pDoc); + } else { + UtUnit aCurrentUnit = getUnitForCell(aIt.GetPos(), pDoc); + if (aFirstUnit.get() != aCurrentUnit) { + return { UnitsStatus::UNITS_INVALID, boost::none }; + } + } + } while (aIt.next()); - while (aIt.next()) { - UtUnit aCurrentUnit = getUnitForCell(aIt.GetPos(), pDoc); - if (aFirstUnit != aCurrentUnit) { - return { UnitsStatus::UNITS_INVALID, boost::none }; } + + return { UnitsStatus::UNITS_VALID, aFirstUnit }; } + case ocProduct: + { + // To avoid having to detect when we get the first unit (i.e. to avoid + // the play with the optinal< UtUnit > as above), we just start with + // the dimensionless Unit 1 and multiply from there. + // This can also be avoided if we implement a combined Unit and Range + // Iterator (see above for more info). + UtUnit aUnit; + UtUnit::createUnit("", aUnit, mpUnitSystem); + + while (aUnitsStack.size() > 0) { + aUnit *= aUnitsStack.top(); + aUnitsStack.pop(); + } - return { UnitsStatus::UNITS_VALID, aFirstUnit }; - } - case ocProduct: - { - UtUnit aUnit = getUnitForCell(aIt.GetPos(), pDoc); - while (aIt.next()) { - aUnit *= getUnitForCell(aIt.GetPos(), pDoc); + if (aIt.first()) { + do { + aUnit *= getUnitForCell(aIt.GetPos(), pDoc); + } while (aIt.next()); + } + + return { UnitsStatus::UNITS_VALID, aUnit }; } - return { UnitsStatus::UNITS_VALID, aUnit }; - } - default: + default: + return { UnitsStatus::UNITS_UNKNOWN, boost::none }; + } + } else { + SAL_INFO("sc.units", "unit verification not supported for opcode: " << nOpCode); return { UnitsStatus::UNITS_UNKNOWN, boost::none }; } + return { UnitsStatus::UNITS_VALID, pOut }; } OUString UnitsImpl::extractUnitStringFromFormat(const OUString& rFormatString) { @@ -469,62 +545,42 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd pArray->Dump(); #endif - stack< UtUnit > aUnitStack; + stack< StackItem > aStack; for (FormulaToken* pToken = pArray->FirstRPN(); pToken != 0; pToken = pArray->NextRPN()) { switch (pToken->GetType()) { case formula::svSingleRef: { - UtUnit pUnit(getUnitForRef(pToken, rFormulaAddress, pDoc)); + UtUnit aUnit(getUnitForRef(pToken, rFormulaAddress, pDoc)); - if (!pUnit.isValid()) { + if (!aUnit.isValid()) { SAL_INFO("sc.units", "no unit returned for scSingleRef, ut_status: " << getUTStatus()); // This only happens in case of parsing (or system) errors. // However maybe we should be returning "unverified" for // unparseable formulas? // (or even have a "can't be verified" state too?) - // see below for more. + // see below for more.x return false; } - aUnitStack.push(pUnit); + aStack.push( { StackItemType::UNITS, aUnit } ); break; } case formula::svDoubleRef: { ScComplexRefData* pDoubleRef = pToken->GetDoubleRef(); - ScRangeList aRangeList(pDoubleRef->toAbs(rFormulaAddress)); - - // Functions operating on DoubleRefs seem to have a variable number of input - // arguments (which will all preceed the opcode in the TokenArray), hence - // we read all the ranges in first before operating on them. - // The appropriate handling of the opcode depends on the specific opcode - // (i.e. identical units needed for sum, but not for multiplication), hence - // we need to handle that opcode here as opposed to as part of the usual flow - // (where we would simply push the units onto the stack). - pToken = pArray->NextRPN(); - while (pToken->GetType() == formula::svDoubleRef) { - pDoubleRef = pToken->GetDoubleRef(); - aRangeList.Append(pDoubleRef->toAbs(rFormulaAddress)); - pToken = pArray->NextRPN(); - } - - if (!pToken) { - // It's possible to have DoubleRef as the last token, e.g the simplest - // example being "=A1:B1" - in this case the formula can't be evaluated - // anyways, so giving up on unit verification is the most sensible option. - // (I.e. the formula ends up spewing out #VALUE to the end-user - there's - // no point in doing any verification here.) - return true; - } - - UnitsResult aResult(getOutputUnitForDoubleRefOpcode(pToken->GetOpCode(), aRangeList, pDoc)); + ScRange aRange = pDoubleRef->toAbs(rFormulaAddress); + aStack.push( { StackItemType::RANGE, aRange } ); + break; + } + case formula::svByte: + { + UnitsResult aResult = getOutputUnitsForOpCode(aStack, pToken, pDoc); switch (aResult.status) { case UnitsStatus::UNITS_INVALID: - SAL_INFO("sc.units", "svDoubleRef opcode unit error detected"); return false; case UnitsStatus::UNITS_UNKNOWN: // Unsupported hence we stop processing. @@ -532,26 +588,12 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd case UnitsStatus::UNITS_VALID: assert(aResult.units); // ensure that we have the optional unit assert(aResult.units->isValid()); - aUnitStack.push(aResult.units.get()); + aStack.push( { StackItemType::UNITS, aResult.units.get() } ); break; } break; } - case formula::svByte: - { - UtUnit pOut = getOutputUnitsForOpCode(aUnitStack, pToken->GetOpCode()); - - // A null unit indicates either invalid units and/or other erronous input - // i.e. is an indication that getOutputUnitsForOpCode failed. - if (pOut.isValid()) { - aUnitStack.push(pOut); - } else { - return false; - } - - break; - } // As far as I can tell this is only used for an actual numerical value // in which case we just want to use it as scaling factor // (for example [m] + [cm]/100 would be a valid operation) @@ -561,7 +603,8 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd UtUnit::createUnit("", aScale, mpUnitSystem); // Get the dimensionless unit 1 aScale = aScale*(pToken->GetDouble()); - aUnitStack.push(aScale); + aStack.push( { StackItemType::UNITS, aScale } ); + break; } default: diff --git a/sc/source/core/units/unitsimpl.hxx b/sc/source/core/units/unitsimpl.hxx index aacf9bb..6abffdd 100644 --- a/sc/source/core/units/unitsimpl.hxx +++ b/sc/source/core/units/unitsimpl.hxx @@ -13,6 +13,7 @@ #include <boost/optional.hpp> #include <boost/scoped_ptr.hpp> #include <boost/shared_ptr.hpp> +#include <boost/variant.hpp> #include <boost/weak_ptr.hpp> #include <formula/opcode.hxx> @@ -41,6 +42,16 @@ namespace test { class UnitsTest; } +enum class StackItemType { + UNITS, + RANGE +}; + +struct StackItem { + StackItemType type; + boost::variant< ScRange, UtUnit > item; +}; + enum class UnitsStatus { UNITS_VALID, UNITS_UNKNOWN, @@ -92,8 +103,7 @@ public: const OUString& rsOldUnit) SAL_OVERRIDE; private: - UnitsResult getOutputUnitsForOpCode(std::stack< UtUnit >& rUnitStack, const OpCode& rOpCode); - UnitsResult getOutputUnitForDoubleRefOpcode(const OpCode& rOpCode, const ScRangeList& rRange, ScDocument* pDoc); + UnitsResult getOutputUnitsForOpCode(std::stack< StackItem >& rStack, const formula::FormulaToken* pToken, ScDocument* pDoc); /** * Find and extract a Unit in the standard header notation, commit ab49005fecee1a28e376b10883b82d88d80eab9d Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Mon Mar 30 19:57:04 2015 +0200 Use UnitsResult for processing of opcodes too. Change-Id: I7bbbf4ff0bcbd01d70fd929cf62537ceec364c83 diff --git a/sc/source/core/units/unitsimpl.cxx b/sc/source/core/units/unitsimpl.cxx index 3a181b5..6057a0e 100644 --- a/sc/source/core/units/unitsimpl.cxx +++ b/sc/source/core/units/unitsimpl.cxx @@ -85,7 +85,7 @@ UnitsImpl::~UnitsImpl() { // (i.e. if udunits can't handle being used across threads)? } -UtUnit UnitsImpl::getOutputUnitsForOpCode(stack< UtUnit >& rUnitStack, const OpCode& rOpCode) { +UnitsResult UnitsImpl::getOutputUnitsForOpCode(stack< UtUnit >& rUnitStack, const OpCode& rOpCode) { UtUnit pOut; auto nOpCode = static_cast<std::underlying_type<const OpCode>::type>(rOpCode); @@ -98,7 +98,7 @@ UtUnit UnitsImpl::getOutputUnitsForOpCode(stack< UtUnit >& rUnitStack, const OpC if (!(rUnitStack.size() >= 1)) { SAL_WARN("sc.units", "no units on stack for unary operation"); - return UtUnit(); + return { UnitsStatus::UNITS_INVALID, boost::none }; } UtUnit pUnit = rUnitStack.top(); @@ -107,7 +107,7 @@ UtUnit UnitsImpl::getOutputUnitsForOpCode(stack< UtUnit >& rUnitStack, const OpC switch (rOpCode) { case ocNot: if (!pUnit.isDimensionless()) { - return UtUnit(); + return { UnitsStatus::UNITS_INVALID, boost::none }; } // We just keep the same unit (in this case no unit) so can // fall through. @@ -132,7 +132,7 @@ UtUnit UnitsImpl::getOutputUnitsForOpCode(stack< UtUnit >& rUnitStack, const OpC SAL_WARN("sc.units", "less than two units on stack when attempting binary operation"); // TODO: what should we be telling the user in this case? Can this even happen (i.e. // should we just be asserting here?) - return UtUnit(); + return { UnitsStatus::UNITS_INVALID, boost::none }; } UtUnit pSecondUnit = rUnitStack.top(); @@ -150,6 +150,7 @@ UtUnit UnitsImpl::getOutputUnitsForOpCode(stack< UtUnit >& rUnitStack, const OpC pOut = pFirstUnit; SAL_INFO("sc.units", "verified equality for unit " << pFirstUnit); } else { + return { UnitsStatus::UNITS_INVALID, boost::none }; // TODO: notify/link UI. } break; @@ -166,11 +167,11 @@ UtUnit UnitsImpl::getOutputUnitsForOpCode(stack< UtUnit >& rUnitStack, const OpC } else { SAL_INFO("sc.units", "unit verification not supported for opcode: " << nOpCode); + return { UnitsStatus::UNITS_UNKNOWN, boost::none }; } // TODO: else if unary, or no params, or ... // TODO: implement further sensible opcode handling - - return pOut; + return { UnitsStatus::UNITS_VALID, pOut }; } class RangeListIterator { diff --git a/sc/source/core/units/unitsimpl.hxx b/sc/source/core/units/unitsimpl.hxx index d6d001c..aacf9bb 100644 --- a/sc/source/core/units/unitsimpl.hxx +++ b/sc/source/core/units/unitsimpl.hxx @@ -92,7 +92,7 @@ public: const OUString& rsOldUnit) SAL_OVERRIDE; private: - UtUnit getOutputUnitsForOpCode(std::stack< UtUnit >& rUnitStack, const OpCode& rOpCode); + UnitsResult getOutputUnitsForOpCode(std::stack< UtUnit >& rUnitStack, const OpCode& rOpCode); UnitsResult getOutputUnitForDoubleRefOpcode(const OpCode& rOpCode, const ScRangeList& rRange, ScDocument* pDoc); /** commit f300d3e5fb406c9cb8df09679144fa8d31751405 Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Mon Mar 30 11:12:42 2015 +0200 Implement verification of sum/product and other opcodes for ranges. Change-Id: I8d0d4cf46bddd585e633b5ee297b6bd7e0a2cf3b diff --git a/sc/qa/unit/units.cxx b/sc/qa/unit/units.cxx index af5fff5..ac1bfa5 100644 --- a/sc/qa/unit/units.cxx +++ b/sc/qa/unit/units.cxx @@ -79,6 +79,11 @@ void UnitsTest::setUp() { mpDoc = &m_xDocShRef->GetDocument(); mpUnitsImpl = UnitsImpl::GetUnits(); + + // Set separators for formulae - for now just argument separators (no matrix separators) + ScFormulaOptions aOptions; + aOptions.SetFormulaSepArg(","); + m_xDocShRef->SetFormulaOptions(aOptions); } void UnitsTest::tearDown() { @@ -183,11 +188,19 @@ void UnitsTest::testUnitVerification() { mpDoc->SetNumberFormat(address, nKeyS); mpDoc->SetValue(address, 3); - // 4th column: 5cm/s + // 4th column: 5cm/s, 10cm/s, 15cm/s address = ScAddress(3, 0, 0); mpDoc->SetNumberFormat(address, nKeyCM_S); mpDoc->SetValue(address, 5); + address.IncRow(); + mpDoc->SetNumberFormat(address, nKeyCM_S); + mpDoc->SetValue(address, 10); + + address.IncRow(); + mpDoc->SetNumberFormat(address, nKeyCM_S); + mpDoc->SetValue(address, 15); + // 5th column: 1m address = ScAddress(4, 0, 0); mpDoc->SetNumberFormat(address, nKeyM); @@ -256,11 +269,81 @@ void UnitsTest::testUnitVerification() { // And scaling doesn't help when adding incompatible units (kg + 100*m) address = ScAddress(0, 13, 0); - mpDoc->SetFormula(address, "=B1+100E1"); + mpDoc->SetFormula(address, "=B1+100*E1"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // Test Ranges: + // SUM("kg") + address.IncRow(); + mpDoc->SetFormula(address, "=SUM(B1:B3)"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // SUM("cm"&"kg") + address.IncRow(); + mpDoc->SetFormula(address, "=SUM(A1:B3)"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // SUM("cm"&"kg") - multiple ranges + address.IncRow(); + // TODO: by default / when testing: separator is...? + mpDoc->SetFormula(address, "=SUM(A1:A3,B1:B3)"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // SUM("cm")+SUM("kg") + address.IncRow(); + mpDoc->SetFormula(address, "=SUM(A1:A3)+SUM(B1:B3)"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // SUM("cm")/SUM("s")+SUM("cm/s") + address.IncRow(); + mpDoc->SetFormula(address, "=SUM(A1:A3)/SUM(C1:C3)+SUM(D1:D3)"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // PRODUCT("cm/","s")+"cm" + address.IncRow(); + mpDoc->SetFormula(address, "=PRODUCT(C1:D1)+A1"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // PRODUCT("cm/","s")+"kg" + address.IncRow(); + mpDoc->SetFormula(address, "=PRODUCT(C1:D1)+B1"); pCell = mpDoc->GetFormulaCell(address); pTokens = pCell->GetCode(); CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + // Do a quick sanity check for all the other supported functions + // The following all expect identically united inputs, and should + // preserve that unit: + std::vector<OUString> aPreservingFuncs{ "SUM", "AVERAGE", "MIN", "MAX" }; + for (const OUString& aFunc: aPreservingFuncs) { + // FOO(cm) + cm + address.IncRow(); + mpDoc->SetFormula(address, "=" + aFunc + "(A1:A2)+A3"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // FOO(cm) + kg + address.IncRow(); + mpDoc->SetFormula(address, "=" + aFunc + "(A1:A2)+B3"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + } } void UnitsTest::testUnitFromFormatStringExtraction() { diff --git a/sc/source/core/units/unitsimpl.cxx b/sc/source/core/units/unitsimpl.cxx index 6d280b4..3a181b5 100644 --- a/sc/source/core/units/unitsimpl.cxx +++ b/sc/source/core/units/unitsimpl.cxx @@ -11,6 +11,7 @@ #include "util.hxx" +#include "dociter.hxx" #include "document.hxx" #include "refdata.hxx" #include "stringutil.hxx" @@ -172,6 +173,82 @@ UtUnit UnitsImpl::getOutputUnitsForOpCode(stack< UtUnit >& rUnitStack, const OpC return pOut; } +class RangeListIterator { +private: + const ScRangeList mRangeList; + ScDocument* mpDoc; + + ScCellIterator mIt; + size_t nCurrentIndex; + +public: + RangeListIterator(ScDocument* pDoc, const ScRangeList& rRangeList) + : + mRangeList(rRangeList), + mpDoc(pDoc), + mIt(pDoc, ScRange()), + nCurrentIndex(0) + { + if (rRangeList.size() > 0) { + mIt = ScCellIterator(pDoc, *rRangeList[0]); + mIt.first(); + } + } + + const ScAddress& GetPos() const { + return mIt.GetPos(); + } + + bool next() { + if (!(mRangeList.size() > 0) || nCurrentIndex >= mRangeList.size()) { + return false; + } + + if (mIt.next()) { + return true; + } else if (++nCurrentIndex < mRangeList.size()) { + mIt = ScCellIterator(mpDoc, *mRangeList[nCurrentIndex]); + mIt.first(); + return true; + } else { + return false; + } + } +}; + +UnitsResult UnitsImpl::getOutputUnitForDoubleRefOpcode(const OpCode& rOpCode, const ScRangeList& rRangeList, ScDocument* pDoc) { + RangeListIterator aIt(pDoc, rRangeList); + + switch (rOpCode) { + case ocSum: + case ocMin: + case ocMax: + case ocAverage: + { + UtUnit aFirstUnit = getUnitForCell(aIt.GetPos(), pDoc); + + while (aIt.next()) { + UtUnit aCurrentUnit = getUnitForCell(aIt.GetPos(), pDoc); + if (aFirstUnit != aCurrentUnit) { + return { UnitsStatus::UNITS_INVALID, boost::none }; + } + } + + return { UnitsStatus::UNITS_VALID, aFirstUnit }; + } + case ocProduct: + { + UtUnit aUnit = getUnitForCell(aIt.GetPos(), pDoc); + while (aIt.next()) { + aUnit *= getUnitForCell(aIt.GetPos(), pDoc); + } + return { UnitsStatus::UNITS_VALID, aUnit }; + } + default: + return { UnitsStatus::UNITS_UNKNOWN, boost::none }; + } +} + OUString UnitsImpl::extractUnitStringFromFormat(const OUString& rFormatString) { // TODO: decide what we do for different subformats? Simplest solution // would be to not allow unit storage for multiple subformats. @@ -413,6 +490,53 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd aUnitStack.push(pUnit); break; } + case formula::svDoubleRef: + { + ScComplexRefData* pDoubleRef = pToken->GetDoubleRef(); + ScRangeList aRangeList(pDoubleRef->toAbs(rFormulaAddress)); + + // Functions operating on DoubleRefs seem to have a variable number of input + // arguments (which will all preceed the opcode in the TokenArray), hence + // we read all the ranges in first before operating on them. + // The appropriate handling of the opcode depends on the specific opcode + // (i.e. identical units needed for sum, but not for multiplication), hence + // we need to handle that opcode here as opposed to as part of the usual flow + // (where we would simply push the units onto the stack). + pToken = pArray->NextRPN(); + while (pToken->GetType() == formula::svDoubleRef) { + pDoubleRef = pToken->GetDoubleRef(); + aRangeList.Append(pDoubleRef->toAbs(rFormulaAddress)); + pToken = pArray->NextRPN(); + } + + if (!pToken) { + // It's possible to have DoubleRef as the last token, e.g the simplest + // example being "=A1:B1" - in this case the formula can't be evaluated + // anyways, so giving up on unit verification is the most sensible option. + // (I.e. the formula ends up spewing out #VALUE to the end-user - there's + // no point in doing any verification here.) + return true; + } + + UnitsResult aResult(getOutputUnitForDoubleRefOpcode(pToken->GetOpCode(), aRangeList, pDoc)); + + + switch (aResult.status) { + case UnitsStatus::UNITS_INVALID: + SAL_INFO("sc.units", "svDoubleRef opcode unit error detected"); + return false; + case UnitsStatus::UNITS_UNKNOWN: + // Unsupported hence we stop processing. + return true; + case UnitsStatus::UNITS_VALID: + assert(aResult.units); // ensure that we have the optional unit + assert(aResult.units->isValid()); + aUnitStack.push(aResult.units.get()); + break; + } + + break; + } case formula::svByte: { UtUnit pOut = getOutputUnitsForOpCode(aUnitStack, pToken->GetOpCode()); diff --git a/sc/source/core/units/unitsimpl.hxx b/sc/source/core/units/unitsimpl.hxx index e1427ed..d6d001c 100644 --- a/sc/source/core/units/unitsimpl.hxx +++ b/sc/source/core/units/unitsimpl.hxx @@ -10,6 +10,7 @@ #ifndef INCLUDED_SC_SOURCE_CORE_UNITS_UNITSIMPL_HXX #define INCLUDED_SC_SOURCE_CORE_UNITS_UNITSIMPL_HXX +#include <boost/optional.hpp> #include <boost/scoped_ptr.hpp> #include <boost/shared_ptr.hpp> #include <boost/weak_ptr.hpp> @@ -20,7 +21,9 @@ #include <udunits2.h> -#include <units.hxx> +#include "rangelst.hxx" +#include "units.hxx" + #include "utunit.hxx" #include <stack> @@ -38,6 +41,22 @@ namespace test { class UnitsTest; } +enum class UnitsStatus { + UNITS_VALID, + UNITS_UNKNOWN, + UNITS_INVALID +}; + +/** + * The result for a given units operation. + * If UNITS_VALID then the resulting unit is also included, + * otherwise units is empty. + */ +struct UnitsResult { + UnitsStatus status; + boost::optional<UtUnit> units; +}; + class UnitsImpl: public Units { friend class test::UnitsTest; @@ -74,6 +93,7 @@ public: private: UtUnit getOutputUnitsForOpCode(std::stack< UtUnit >& rUnitStack, const OpCode& rOpCode); + UnitsResult getOutputUnitForDoubleRefOpcode(const OpCode& rOpCode, const ScRangeList& rRange, ScDocument* pDoc); /** * Find and extract a Unit in the standard header notation, commit 9b3ca1ea7ba35f2c1404d24419edde2d7b02ecc7 Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Mon Mar 30 09:50:45 2015 +0200 Move declarations to more sensible place. Change-Id: Ieb60b2a263a4d700f366b0aae7128e5a66ac4205 diff --git a/sc/source/core/units/unitsimpl.hxx b/sc/source/core/units/unitsimpl.hxx index 91730f7..e1427ed 100644 --- a/sc/source/core/units/unitsimpl.hxx +++ b/sc/source/core/units/unitsimpl.hxx @@ -74,8 +74,6 @@ public: private: UtUnit getOutputUnitsForOpCode(std::stack< UtUnit >& rUnitStack, const OpCode& rOpCode); - OUString extractUnitStringFromFormat(const OUString& rFormatString); - OUString extractUnitStringForCell(const ScAddress& rAddress, ScDocument* pDoc); /** * Find and extract a Unit in the standard header notation, @@ -101,6 +99,9 @@ private: bool extractUnitFromHeaderString(const OUString& rHeader, UtUnit& aUnit, OUString& sUnitString); + OUString extractUnitStringFromFormat(const OUString& rFormatString); + OUString extractUnitStringForCell(const ScAddress& rAddress, ScDocument* pDoc); + /** * Retrieve the units for a given cell. This probes based on the usual rules * for cell annotation/column header. commit 640e4e47758baebdef385f135ab6a07b8c9ada36 Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Mon Mar 30 09:50:05 2015 +0200 Implement UtUnit::operator*= Change-Id: I2733154c3d7c42f3af273a0267158e8fa5957c23 diff --git a/sc/source/core/units/utunit.hxx b/sc/source/core/units/utunit.hxx index 24225dd..c137f67 100644 --- a/sc/source/core/units/utunit.hxx +++ b/sc/source/core/units/utunit.hxx @@ -90,6 +90,11 @@ public: return UtUnit(ut_multiply(this->get(), rUnit.get())); } + UtUnit& operator*=(const UtUnit& rUnit) { + reset(ut_multiply(this->get(), rUnit.get())); + return *this; + } + /** * Multiply a unit by a given constant. * This scales the unit by the inverse of that constant, commit c4e7c77021d92c22e9cec7733f5be6a22de6bfaa Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Mon Mar 30 09:49:27 2015 +0200 Split getUnitForRef into mapping and retrieval. We will need getUnitForCell when verifying formulas operating on ranges (where we need to iterate over cells in a range, and retrieve those cells units - as opposed to retrieving the unit for a reference in a token array). Change-Id: I6652057bcb3ee42becb4c5425b06ce544cd400a6 diff --git a/sc/source/core/units/unitsimpl.cxx b/sc/source/core/units/unitsimpl.cxx index 9492dd1..6d280b4 100644 --- a/sc/source/core/units/unitsimpl.cxx +++ b/sc/source/core/units/unitsimpl.cxx @@ -314,19 +314,8 @@ bool UnitsImpl::extractUnitFromHeaderString(const OUString& rsHeader, UtUnit& aU return false; } -UtUnit UnitsImpl::getUnitForRef(FormulaToken* pToken, const ScAddress& rFormulaAddress, - ScDocument* pDoc) { - assert(pToken->GetType() == formula::svSingleRef); - - ScSingleRefData* pRef = pToken->GetSingleRef(); - assert(pRef); - - // Addresses can/will be relative to the formula, for extracting - // units however we will need to get the absolute address (i.e. - // by adding the current address to the relative formula address). - const ScAddress aInputAddress = pRef->toAbs( rFormulaAddress ); - - OUString sUnitString = extractUnitStringForCell(aInputAddress, pDoc); +UtUnit UnitsImpl::getUnitForCell(const ScAddress& rCellAddress, ScDocument* pDoc) { + OUString sUnitString = extractUnitStringForCell(rCellAddress, pDoc); UtUnit aUnit; if (sUnitString.getLength() > 0 && @@ -336,15 +325,31 @@ UtUnit UnitsImpl::getUnitForRef(FormulaToken* pToken, const ScAddress& rFormulaA OUString aHeaderUnitString; // Unused -- passed by reference below ScAddress aHeaderAddress; // Unused too - UtUnit aHeaderUnit = findHeaderUnitForCell(aInputAddress, pDoc, aHeaderUnitString, aHeaderAddress); + UtUnit aHeaderUnit = findHeaderUnitForCell(rCellAddress, pDoc, aHeaderUnitString, aHeaderAddress); if (aHeaderUnit.isValid()) return aHeaderUnit; - SAL_INFO("sc.units", "no unit obtained for token at cell " << aInputAddress.GetColRowString()); + SAL_INFO("sc.units", "no unit obtained for token at cell " << rCellAddress.GetColRowString()); // We return the dimensionless unit 1 if we don't find any other data suggesting a unit. UtUnit::createUnit("", aUnit, mpUnitSystem); return aUnit; + +} + +UtUnit UnitsImpl::getUnitForRef(FormulaToken* pToken, const ScAddress& rFormulaAddress, + ScDocument* pDoc) { + assert(pToken->GetType() == formula::svSingleRef); + + ScSingleRefData* pRef = pToken->GetSingleRef(); + assert(pRef); + + // Addresses can/will be relative to the formula, for extracting + // units however we will need to get the absolute address (i.e. + // by adding the current address to the relative formula address). + const ScAddress aCellAddress = pRef->toAbs( rFormulaAddress ); + + return getUnitForCell(aCellAddress, pDoc); } UtUnit UnitsImpl::findHeaderUnitForCell(const ScAddress& rCellAddress, diff --git a/sc/source/core/units/unitsimpl.hxx b/sc/source/core/units/unitsimpl.hxx index 1e8ac8f..91730f7 100644 --- a/sc/source/core/units/unitsimpl.hxx +++ b/sc/source/core/units/unitsimpl.hxx @@ -101,6 +101,12 @@ private: bool extractUnitFromHeaderString(const OUString& rHeader, UtUnit& aUnit, OUString& sUnitString); + /** + * Retrieve the units for a given cell. This probes based on the usual rules + * for cell annotation/column header. + * Retrieving units for a formula cell is not yet supported. + */ + UtUnit getUnitForCell(const ScAddress& rCellAddress, ScDocument* pDoc); UtUnit getUnitForRef(formula::FormulaToken* pToken, const ScAddress& rFormulaAddress, ScDocument* pDoc); commit eccddddee5b9a03c0f32ca8d273d3cf6e5a4709a Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Mon Mar 30 09:46:36 2015 +0200 Typo in comment Change-Id: Ic6755b950f6b26546072afa82a9ad68caf2becc8 diff --git a/sc/source/core/units/unitsimpl.cxx b/sc/source/core/units/unitsimpl.cxx index 06cd4f3..9492dd1 100644 --- a/sc/source/core/units/unitsimpl.cxx +++ b/sc/source/core/units/unitsimpl.cxx @@ -257,7 +257,7 @@ bool UnitsImpl::findUnitInStandardHeader(const OUString& rsHeader, UtUnit& aUnit bool UnitsImpl::findFreestandingUnitInHeader(const OUString& rsHeader, UtUnit& aUnit, OUString& sUnitString) { - // We just split the string and test whether each token is either a valid unit in it's own right, + // We just split the string and test whether each token is either a valid unit in its own right, // or is an operator that could glue together multiple units (i.e. multiplication/division). // This is sufficient for when there are spaces between elements composing the unit, and none // of the individual elements starts or begins with an operator. commit 9cdb8f9dc8cdde7b8f26a143a3ad105147d82545 Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Sat Mar 21 06:47:24 2015 +0100 Handle constants in formulas for verification too. Change-Id: Ib1b07e6dabf961726b3411b5ff00c67862d95d03 diff --git a/sc/qa/unit/units.cxx b/sc/qa/unit/units.cxx index a0d5fa1..af5fff5 100644 --- a/sc/qa/unit/units.cxx +++ b/sc/qa/unit/units.cxx @@ -125,7 +125,7 @@ void UnitsTest::testUnitVerification() { mpDoc->EnsureTable(0); SvNumberFormatter* pFormatter = mpDoc->GetFormatTable(); - sal_uInt32 nKeyCM, nKeyKG, nKeyS, nKeyCM_S; + sal_uInt32 nKeyCM, nKeyM, nKeyKG, nKeyS, nKeyCM_S; // Used to return position of error in input string for PutEntry // -- not needed here. @@ -133,6 +133,8 @@ void UnitsTest::testUnitVerification() { short nType = css::util::NumberFormat::DEFINED; + OUString sM = "#\"m\""; + pFormatter->PutEntry(sM, nCheckPos, nType, nKeyM); OUString sCM = "#\"cm\""; pFormatter->PutEntry(sCM, nCheckPos, nType, nKeyCM); OUString sKG = "#\"kg\""; @@ -186,6 +188,11 @@ void UnitsTest::testUnitVerification() { mpDoc->SetNumberFormat(address, nKeyCM_S); mpDoc->SetValue(address, 5); + // 5th column: 1m + address = ScAddress(4, 0, 0); + mpDoc->SetNumberFormat(address, nKeyM); + mpDoc->SetValue(address, 1); + ScFormulaCell* pCell; ScTokenArray* pTokens; @@ -223,6 +230,37 @@ void UnitsTest::testUnitVerification() { pCell = mpDoc->GetFormulaCell(address); pTokens = pCell->GetCode(); CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // Test that addition of scaled units works (cm + 100*m) + address = ScAddress(0, 10, 0); + mpDoc->SetFormula(address, "=A1+100*E1"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + // 10cm + 100*1m = 110cm + CPPUNIT_ASSERT(mpDoc->GetValue(address) == 110); + + // But addition of them unscaled fails (cm + m) + address = ScAddress(0, 11, 0); + mpDoc->SetFormula(address, "=A1+E1"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // As does wrong scaling (cm+m/50) + address = ScAddress(0, 12, 0); + mpDoc->SetFormula(address, "=A1+E1/50"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + + // And scaling doesn't help when adding incompatible units (kg + 100*m) + address = ScAddress(0, 13, 0); + mpDoc->SetFormula(address, "=B1+100E1"); + pCell = mpDoc->GetFormulaCell(address); + pTokens = pCell->GetCode(); + CPPUNIT_ASSERT(!mpUnitsImpl->verifyFormula(pTokens, address, mpDoc)); + } void UnitsTest::testUnitFromFormatStringExtraction() { diff --git a/sc/source/core/units/unitsimpl.cxx b/sc/source/core/units/unitsimpl.cxx index 58f4d14..06cd4f3 100644 --- a/sc/source/core/units/unitsimpl.cxx +++ b/sc/source/core/units/unitsimpl.cxx @@ -422,6 +422,18 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd break; } + // As far as I can tell this is only used for an actual numerical value + // in which case we just want to use it as scaling factor + // (for example [m] + [cm]/100 would be a valid operation) + case formula::svDouble: + { + UtUnit aScale; + UtUnit::createUnit("", aScale, mpUnitSystem); // Get the dimensionless unit 1 + aScale = aScale*(pToken->GetDouble()); + + aUnitStack.push(aScale); + break; + } default: // We can't parse any other types of tokens yet, so assume that the formula // was correct. commit 052c19c26fdc152646a5c476a957aa2b0be5e82e Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Sat Mar 21 06:46:15 2015 +0100 Refactor unit extraction to separate local/header unit extraction Change-Id: I46eb09fcbb4d83fc58140cda7fada2bf287b0a65 diff --git a/sc/source/core/units/unitsimpl.cxx b/sc/source/core/units/unitsimpl.cxx index 5c9b28c..58f4d14 100644 --- a/sc/source/core/units/unitsimpl.cxx +++ b/sc/source/core/units/unitsimpl.cxx @@ -208,7 +208,10 @@ OUString UnitsImpl::extractUnitStringForCell(const ScAddress& rAddress, ScDocume return extractUnitStringFromFormat(rFormatString); } -bool UnitsImpl::extractUnitFromHeaderString(const OUString& rString, UtUnit& aUnit, OUString& sUnitString) { +bool UnitsImpl::findUnitInStandardHeader(const OUString& rsHeader, UtUnit& aUnit, OUString& sUnitString) { + // TODO: we should do a sanity check that there's only one such unit though (and fail if there are multiple). + // Since otherwise there's no way for us to know which unit is the intended one, hence we need to get + // the user to deconfuse us by correcting their header to only contain the intended unit. com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> xContext = comphelper::getProcessComponentContext(); uno::Reference<lang::XMultiServiceFactory> xFactory(xContext->getServiceManager(), uno::UNO_QUERY_THROW); @@ -222,18 +225,14 @@ bool UnitsImpl::extractUnitFromHeaderString(const OUString& rString, UtUnit& aUn aOptions.algorithmType = util::SearchAlgorithms_REGEXP ; aOptions.searchFlag = util::SearchFlags::ALL_IGNORE_CASE; - // 1. We try to check for units contained within square brackets - // TODO: we should do a sanity check that there's only one such unit though (and fail if there are multiple). - // Since otherwise there's no way for us to know which unit is the intended one, hence we need to get - // the user to deconfuse us by correcting their header to only contain the intended unit. aOptions.searchString = "\\[([^\\]]+)\\]"; // Grab the contents between [ and ]. xSearch->setOptions( aOptions ); util::SearchResult aResult; - sal_Int32 nStartPosition = rString.getLength(); + sal_Int32 nStartPosition = rsHeader.getLength(); while (nStartPosition) { // Search from the back since units are more likely to be at the end of the header. - aResult = xSearch->searchBackward(rString, nStartPosition, 0); + aResult = xSearch->searchBackward(rsHeader, nStartPosition, 0); // We have either 0 items (no match), or 2 (matched string + the group within) if (aResult.subRegExpressions != 2) { @@ -243,7 +242,7 @@ bool UnitsImpl::extractUnitFromHeaderString(const OUString& rString, UtUnit& aUn // i.e. startOffset is the last character of the intended substring, endOffset the first character. // We specifically grab the offsets for the first actual regex group, which are stored in [1], the indexes // at [0] represent the whole matched string (i.e. including square brackets). - sUnitString = rString.copy(aResult.endOffset[1], aResult.startOffset[1] - aResult.endOffset[1]); + sUnitString = rsHeader.copy(aResult.endOffset[1], aResult.startOffset[1] - aResult.endOffset[1]); if (UtUnit::createUnit(sUnitString, aUnit, mpUnitSystem)) { return true; @@ -252,17 +251,26 @@ bool UnitsImpl::extractUnitFromHeaderString(const OUString& rString, UtUnit& aUn nStartPosition = aResult.endOffset[0]; } } + sUnitString.clear(); + return false; +} + - // 2. We try to check for any free-standing units by splitting the string and testing each split - const sal_Int32 nTokenCount = comphelper::string::getTokenCount(rString, ' '); +bool UnitsImpl::findFreestandingUnitInHeader(const OUString& rsHeader, UtUnit& aUnit, OUString& sUnitString) { + // We just split the string and test whether each token is either a valid unit in it's own right, + // or is an operator that could glue together multiple units (i.e. multiplication/division). + // This is sufficient for when there are spaces between elements composing the unit, and none + // of the individual elements starts or begins with an operator. // There's an inherent limit to how well we can cope with various spacing issues here without // a ton of computational complexity. - // E.g. by parsing in this way we might end up with unmatched parentheses which udunits won't like. - // for now operators have to be completely freestanding i.e. "cm / kg" or completely within a valid unit string e.g. "cm/kg" + // E.g. by parsing in this way we might end up with unmatched parentheses which udunits won't like + // (thus rejecting the unit) etc. + + const sal_Int32 nTokenCount = comphelper::string::getTokenCount(rsHeader, ' '); const OUString sOperators = "/*"; // valid sUnitString.clear(); for (sal_Int32 nToken = 0; nToken < nTokenCount; nToken++) { - OUString sToken = rString.getToken(nToken, ' '); + OUString sToken = rsHeader.getToken(nToken, ' '); UtUnit aTestUnit; if (UtUnit::createUnit(sToken, aTestUnit, mpUnitSystem) || // Only test for a separator character if we have already got something in our string, as @@ -285,6 +293,20 @@ bool UnitsImpl::extractUnitFromHeaderString(const OUString& rString, UtUnit& aUn if (sUnitString.getLength() && UtUnit::createUnit(sUnitString, aUnit, mpUnitSystem)) { return true; } + sUnitString.clear(); + return false; +} + +bool UnitsImpl::extractUnitFromHeaderString(const OUString& rsHeader, UtUnit& aUnit, OUString& sUnitString) { + // 1. Ideally we have units in a 'standard' format, i.e. enclose in square brackets: + if (findUnitInStandardHeader(rsHeader, aUnit, sUnitString)) { + return true; + } + + // 2. But if not we check for free-standing units + if (findFreestandingUnitInHeader(rsHeader, aUnit, sUnitString)) { + return true; + } // 3. Give up aUnit = UtUnit(); // assign invalid diff --git a/sc/source/core/units/unitsimpl.hxx b/sc/source/core/units/unitsimpl.hxx index 5616379..1e8ac8f 100644 --- a/sc/source/core/units/unitsimpl.hxx +++ b/sc/source/core/units/unitsimpl.hxx @@ -76,7 +76,31 @@ private: UtUnit getOutputUnitsForOpCode(std::stack< UtUnit >& rUnitStack, const OpCode& rOpCode); OUString extractUnitStringFromFormat(const OUString& rFormatString); OUString extractUnitStringForCell(const ScAddress& rAddress, ScDocument* pDoc); - bool extractUnitFromHeaderString(const OUString& rString, UtUnit& aUnit, OUString& sUnitString); + + /** + * Find and extract a Unit in the standard header notation, + * i.e. a unit enclose within square brackets (e.g. "length [cm]". + * + * @return true if such a unit is found. + */ + bool findUnitInStandardHeader(const OUString& rHeader, UtUnit& aUnit, OUString& sUnitString); + /** + * Find and extract a freestanding Unit from a header string. + * This includes strings such as "speed m/s", "speed m / s", + * "speed (m/s)" etc. + * This is (for now) only a fallback for the case that the user hasn't defined + * units in the standard notation, and can only handle a limited number + * of ways in which units could be written in a string. + * It may turn out that in practice this method should be extended to support + * more permutations of the same unit, but this should at least cover the most + * obvious cases. + * + * @ return true if a unit is found. + */ + bool findFreestandingUnitInHeader(const OUString& rHeader, UtUnit& aUnit, OUString& sUnitString); + + bool extractUnitFromHeaderString(const OUString& rHeader, UtUnit& aUnit, OUString& sUnitString); + UtUnit getUnitForRef(formula::FormulaToken* pToken, const ScAddress& rFormulaAddress, ScDocument* pDoc); commit b265b5d2d35aacb617c210c19df64a68e9b42320 Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Sat Mar 21 06:42:48 2015 +0100 Implement UtUnit::operator* for constants Change-Id: I9c44f0a4d0b78c3eb3262bd55d8304b7b07460e2 diff --git a/sc/source/core/units/utunit.hxx b/sc/source/core/units/utunit.hxx index 058891a..24225dd 100644 --- a/sc/source/core/units/utunit.hxx +++ b/sc/source/core/units/utunit.hxx @@ -90,6 +90,19 @@ public: return UtUnit(ut_multiply(this->get(), rUnit.get())); } + /** + * Multiply a unit by a given constant. + * This scales the unit by the inverse of that constant, + * e.g. 100*m is equivalent to cm, hence + * the unit needs to be scaled by 1/100. + * (This is implemented in this way so that we can simply + * multiply units by any constants present in a formula + * in a natural way.) + */ + UtUnit operator*(const double nMultiplier) { + return UtUnit(ut_scale(1/nMultiplier, this->get())); + } + UtUnit operator/(const UtUnit& rUnit) { // the parameter is the right hand side value in the operation, // i.e. we are working with this / rUnit. commit 669f6861c6d9f2e60ea51accb1cc4c1fc99c66b2 Author: Andrzej Hunt <andrzej.h...@collabora.com> Date: Wed Mar 18 12:28:33 2015 +0000 Turn while into for for clarity. Change-Id: I6e55441c900b64af014fe902c9c8bf928aa7e31a diff --git a/sc/source/core/units/unitsimpl.cxx b/sc/source/core/units/unitsimpl.cxx index 427aec3..5c9b28c 100644 --- a/sc/source/core/units/unitsimpl.cxx +++ b/sc/source/core/units/unitsimpl.cxx @@ -366,9 +366,7 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd stack< UtUnit > aUnitStack; - FormulaToken* pToken = pArray->FirstRPN(); - - while (pToken != 0) { + for (FormulaToken* pToken = pArray->FirstRPN(); pToken != 0; pToken = pArray->NextRPN()) { switch (pToken->GetType()) { case formula::svSingleRef: { @@ -409,8 +407,6 @@ bool UnitsImpl::verifyFormula(ScTokenArray* pArray, const ScAddress& rFormulaAdd SAL_WARN("sc.units", "Unrecognised token type " << pToken->GetType()); return true; } - - pToken = pArray->NextRPN(); } // TODO: only fail if actual parsing fails? _______________________________________________ Libreoffice-commits mailing list libreoffice-comm...@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice-commits