[PATCH]fdo 50488 added calc formula XOR as defined in ODFF1.2
Eike Rathke
erack at redhat.com
Fri Jun 8 06:02:36 PDT 2012
Hi Winfried,
On Thursday, 2012-06-07 14:51:28 +0200, Winfried Donkers wrote:
> This time the patch is the one that covers XOR.
Looks good in general, some nitpicks (as usual ;-)
> --- a/formula/inc/formula/compiler.hrc
> +++ b/formula/inc/formula/compiler.hrc
> @@ -89,7 +89,8 @@
> #define SC_OPCODE_INTERSECT 54
> #define SC_OPCODE_UNION 55
> #define SC_OPCODE_RANGE 56
> -#define SC_OPCODE_STOP_BIN_OP 57
> +#define SC_OPCODE_XOR 57
> +#define SC_OPCODE_STOP_BIN_OP 58
Having AND and OR as binary operators is a legacy we should not follow
with new functions like XOR. It's necessary to load existing documents
that use this inlining, but further work even is required to store such
in ODF compliance as the inlining operators are not defined there.
So just add the new opcode to the 2-and-more-parameters section.
> --- a/formula/source/core/api/FormulaCompiler.cxx
> +++ b/formula/source/core/api/FormulaCompiler.cxx
> @@ -1128,6 +1129,7 @@ void FormulaCompiler::Factor()
> || eOp == ocMacro
> || eOp == ocAnd
> || eOp == ocOr
> + || eOp == ocXor
> || eOp == ocBad
> || ( eOp >= ocInternalBegin && eOp <= ocInternalEnd )
> || (bCompileForFAP && ((eOp == ocIf) || (eOp == ocChose)))
Then adding ocXor is unnecessary there.
> @@ -1440,7 +1442,7 @@ OpCode FormulaCompiler::Expression()
> return ocStop; //! generate token instead?
> }
> NotLine();
> - while (pToken->GetOpCode() == ocAnd || pToken->GetOpCode() == ocOr)
> + while (pToken->GetOpCode() == ocAnd || pToken->GetOpCode() == ocOr || pToken->GetOpCode() == ocXor)
> {
> FormulaTokenRef p = pToken;
> pToken->SetByte( 2 ); // 2 parameters!
And there.
> @@ -1617,7 +1619,7 @@ FormulaToken* FormulaCompiler::CreateStringFromToken( rtl::OUStringBuffer& rBuff
> bool bSpaces = false;
> FormulaToken* t = pTokenP;
> OpCode eOp = t->GetOpCode();
> - if( eOp >= ocAnd && eOp <= ocOr )
> + if( eOp >= ocAnd && eOp <= ocXor )
> {
> // AND, OR infix?
> if ( bAllowArrAdvance )
And there.
> @@ -1822,8 +1824,8 @@ OpCode FormulaCompiler::NextToken()
> else
> {
> // Before an operator there must not be another operator, with the
> - // exception of AND and OR.
> - if ( eOp != ocAnd && eOp != ocOr &&
> + // exception of AND, OR and XOR.
> + if ( eOp != ocAnd && eOp != ocOr && eOp != ocXor &&
> (SC_OPCODE_START_BIN_OP <= eOp && eOp < SC_OPCODE_STOP_BIN_OP )
> && (eLastOp == ocOpen || eLastOp == ocSep ||
> (SC_OPCODE_START_BIN_OP <= eLastOp && eLastOp < SC_OPCODE_STOP_UN_OP)))
And there.
> --- a/formula/source/core/api/token.cxx
> +++ b/formula/source/core/api/token.cxx
> @@ -90,7 +90,7 @@ bool FormulaToken::IsFunction() const
> || (SC_OPCODE_START_2_PAR <= eOp && eOp < SC_OPCODE_STOP_2_PAR) // x parameters (cByte==0 in
> // FuncAutoPilot)
> || eOp == ocMacro || eOp == ocExternal // macros, AddIns
> - || eOp == ocAnd || eOp == ocOr // former binary, now x parameters
> + || eOp == ocAnd || eOp == ocOr || eOp == ocXor // former binary, now x parameters
> || eOp == ocNot || eOp == ocNeg // unary but function
> || (eOp >= ocInternalBegin && eOp <= ocInternalEnd) // internal
> ));
And there.
;-)
> --- a/sc/inc/helpids.h
> +++ b/sc/inc/helpids.h
> @@ -88,7 +88,6 @@
> #define HID_SC_FORM_ARGS "SC_HID_SC_FORM_ARGS"
> #define HID_SCPAGE_SORT_FIELDS "SC_HID_SCPAGE_SORT_FIELDS"
> #define HID_SCPAGE_SORT_OPTIONS "SC_HID_SCPAGE_SORT_OPTIONS"
> -#define HID_SCPAGE_SORTKEY_FIELDS "SC_HID_SCPAGE_SORTKEY_FIELDS"
Huh? Accidentally hit the delete line key in your editor?
> --- a/sc/source/core/tool/interpr1.cxx
> +++ b/sc/source/core/tool/interpr1.cxx
> +void ScInterpreter::ScXor()
> [...]
> + short nRes = 0;
Using a short here and incrementing it for values!=0 may easily overflow
if ranges are passed. A better approach would be to use the C++ ^ xor
operator, e.g.
short nRes = 0;
nRes ^= (PopDouble() != 0.0);
> --- a/sc/source/core/tool/parclass.cxx
> +++ b/sc/source/core/tool/parclass.cxx
> @@ -498,6 +499,7 @@ void ScParameterClassification::GenerateDocumentation()
> {
> case ocAnd:
> case ocOr:
> + case ocXor:
> aToken.SetByte(1); // (r1)AND(r2) --> AND( r1, ...)
> break;
> default:
This is also unnecessary if the opcode is not allowed as binary
inline operator.
> diff --git a/sc/source/filter/oox/formulabase.cxx b/sc/source/filter/oox/formulabase.cxx
> index b80dbfa..a5276eb 100644
> --- a/sc/source/filter/oox/formulabase.cxx
> +++ b/sc/source/filter/oox/formulabase.cxx
> @@ -669,6 +669,7 @@ static const FunctionData saFuncTableBiff5[] =
> { 0, "DATESTRING", 352, 352, 1, 1, V, { VR }, FUNCFLAG_IMPORTONLY }, // not supported in Calc, missing in OOXML spec
> { 0, "NUMBERSTRING", 353, 353, 2, 2, V, { VR }, FUNCFLAG_IMPORTONLY }, // not supported in Calc, missing in OOXML spec
> { "ROMAN", "ROMAN", 354, 354, 1, 2, V, { VR }, 0 },
> + { "XOR", "XOR", 355, 355, 1, MX, V, { RX }, 0 },
This does not work. Excel doesn't know the XOR function so there is no
function identifier available. Inventing a value like 355 also does not
work, the values have to be those that Excel uses. So, ...
> @@ -762,7 +763,6 @@ static const FunctionData saFuncTableOdf[] =
> { "SKEWP", 0, NOID, NOID, 1, MX, V, { RX }, FUNCFLAG_MACROCALLODF },
> { "UNICHAR", 0, NOID, NOID, 1, 1, V, { VR }, FUNCFLAG_MACROCALLODF },
> { "UNICODE", 0, NOID, NOID, 1, 1, V, { VR }, FUNCFLAG_MACROCALLODF },
> - { "XOR", 0, NOID, NOID, 1, MX, V, { RX }, FUNCFLAG_MACROCALLODF }
... just leave the function in that section then at least Calc can use
it if the .xls is reopened.
With a few changes I think we'd have a working implementation. Please
try and test.
Btw, instead of reusing fdo#50488 for all new implementations I'd prefer
if we created a new bug once we have a function ready and make fdo#50488
depend on the new bug to easily get a list of solutions. Otherwise when
committing a change with fdo#50488 in the commit summary we'd add
a target to that bug that wouldn't match the real release when a new
function will be available.
Keep up the good work :) I'm looking forward ot the corrrected patch.
Eike
--
LibreOffice Calc developer. Number formatter stricken i18n transpositionizer.
GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20120608/894603ea/attachment.pgp>
More information about the LibreOffice
mailing list