Re: [PATCH]fdo 50488 added calc formula XOR as defined in ODFF1.2
Hi Winfried, On Friday, 2012-06-08 17:08:38 +0200, Winfried Donkers wrote: > >Huh? Accidentally hit the delete line key in your editor? > No, I did a git pull -r whilst working on the code and forgot to update this > file. Should have seen it, though. Do you know about the git stash command? Seems that could solve such steps in your workflow. See git stash --help > >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, ... > >... just leave the function in that section then at least Calc can use > >it if the .xls is reopened. > Here I couldn't check Excel and guessed (wrongly) that Excel would have an > XOR function. Well, yes, but even if it did the correct function identifier would have to be used, not an invented one. For the existing entries in those lists I trust the original author that if an entry for binary file format doesn't exist then Excel really doesn't know that ;-) > All right. I created bug 50882 for the XOR function and will create > other bug for other functions (I hope I entered the dependancy > correct). Yep, fine, also thanks for adding me on Cc. Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD pgpkkTJ4mMwy7.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
RE: [PATCH]fdo 50488 added calc formula XOR as defined in ODFF1.2
Hi Eike, >Huh? Accidentally hit the delete line key in your editor? No, I did a git pull -r whilst working on the code and forgot to update this file. Should have seen it, though. >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); True, I followed the ODFF definition too literally. repeated XORing is identical and more in line with the code in ScAnd() and ScOr(). >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, ... >... just leave the function in that section then at least Calc can use >it if the .xls is reopened. Here I couldn't check Excel and guessed (wrongly) that Excel would have an XOR function. >With a few changes I think we'd have a working implementation. Please >try and test. I will build and test with the changes you propose and submit a new patch. >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. All right. I created bug 50882 for the XOR function and will create other bug for other functions (I hope I entered the dependancy correct). >Keep up the good work :) I'm looking forward ot the corrrected patch. I'll do my best :) Winfried ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH]fdo 50488 added calc formula XOR as defined in ODFF1.2
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_OP57
> +#define SC_OPCODE_XOR57
> +#define SC_OPCODE_STOP_BIN_OP58
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;
>
Re: [PATCH]fdo 50488 added calc formula XOR as defined in ODFF1.2
Hi Michael, On Friday, 2012-06-08 10:45:08 +0100, Michael Meeks wrote: > On Wed, 2012-06-06 at 17:32 +0200, Eike Rathke wrote: > > Argh, I missed to reply to you in the bug that XOR is also in the said > > CWS we still hope to be able to integrate back from OOo times. So you > > duplicated some work, my bad. > > Is that CWS already merged into AOOi ? if it is not, then the hope of > getting it under an acceptable license seems slight at the moment; there > is no agreed mechanism for that that I'm aware of even. I just wrote a mail on ooo-dev to bring that on the radar again.. > I'd hesitate to > block useful feature work on the hope of that :-) That's why I stated I'd commit Winfried's patch anyway if it's ok. We'd have clashes then in case we integrated the CWS changes, but that's just a little work to sort out. Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD pgpR9wesk5QBk.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH]fdo 50488 added calc formula XOR as defined in ODFF1.2
On Wed, 2012-06-06 at 17:32 +0200, Eike Rathke wrote: > Argh, I missed to reply to you in the bug that XOR is also in the said > CWS we still hope to be able to integrate back from OOo times. So you > duplicated some work, my bad. Is that CWS already merged into AOOi ? if it is not, then the hope of getting it under an acceptable license seems slight at the moment; there is no agreed mechanism for that that I'm aware of even. I'd hesitate to block useful feature work on the hope of that :-) ATB, Michael. -- michael.me...@suse.com <><, Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH]fdo 50488 added calc formula XOR as defined in ODFF1.2
Hi Winfried, On Thursday, 2012-06-07 14:51:28 +0200, Winfried Donkers wrote: > This time the patch is the one that covers XOR. Thanks, I'll take a look at it tomorrow. Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD pgp5itROfuOjn.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
RE: [PATCH]fdo 50488 added calc formula XOR as defined in ODFF1.2
>> Attached patch adds formula XOR to calc.
>> Possibly the function ought to be included in one or more arrays of
>> functions that are or are not known in Excel, Lotus, Qpro.
> [...] However, you attached a patch for DATEDIF instead of XOR ;-)
This time the patch is the one that covers XOR.
Winfried
From 45326af955b020cc861148c7c1c95449dab858f4 Mon Sep 17 00:00:00 2001
From: Winfried Donkers
Date: Wed, 6 Jun 2012 16:33:38 +0200
Subject: [PATCH] fdo#50488 added calc formula XOR as in ODFF1.2
Change-Id: I89c3267d479d5f034ad25ff879d1df8f90b84a55
---
formula/inc/formula/compiler.hrc |3 +-
formula/inc/formula/opcode.hxx |1 +
formula/source/core/api/FormulaCompiler.cxx| 10 ++-
formula/source/core/api/token.cxx |2 +-
formula/source/core/resource/core_resource.src |5 +
sc/inc/helpids.h |2 +-
sc/qa/unit/ucalc.cxx |1 +
sc/source/core/inc/interpre.hxx|1 +
sc/source/core/tool/interpr1.cxx | 105
sc/source/core/tool/interpr4.cxx |1 +
sc/source/core/tool/parclass.cxx |2 +
sc/source/filter/oox/formulabase.cxx |2 +-
sc/source/ui/src/scfuncs.src | 24 ++
sc/source/ui/vba/vbawsfunction.cxx |2 +-
sc/util/hidother.src |1 +
15 files changed, 153 insertions(+), 9 deletions(-)
diff --git a/formula/inc/formula/compiler.hrc b/formula/inc/formula/compiler.hrc
index a2d4bb5..54ded84 100644
--- 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_OP57
+#define SC_OPCODE_XOR57
+#define SC_OPCODE_STOP_BIN_OP58
/* NOTE: binary and unary operators must be in sequence for compiler! */
diff --git a/formula/inc/formula/opcode.hxx b/formula/inc/formula/opcode.hxx
index a1543dd..16b12ec 100644
--- a/formula/inc/formula/opcode.hxx
+++ b/formula/inc/formula/opcode.hxx
@@ -89,6 +89,7 @@ enum OpCodeEnum
ocGreaterEqual = SC_OPCODE_GREATER_EQUAL,
ocAnd = SC_OPCODE_AND,
ocOr= SC_OPCODE_OR,
+ocXor = SC_OPCODE_XOR,
ocIntersect = SC_OPCODE_INTERSECT,
ocUnion = SC_OPCODE_UNION,
ocRange = SC_OPCODE_RANGE,
diff --git a/formula/source/core/api/FormulaCompiler.cxx b/formula/source/core/api/FormulaCompiler.cxx
index 9e82422..e2de107 100644
--- a/formula/source/core/api/FormulaCompiler.cxx
+++ b/formula/source/core/api/FormulaCompiler.cxx
@@ -76,6 +76,7 @@ short lcl_GetRetFormat( OpCode eOpCode )
case ocGreaterEqual:
case ocAnd:
case ocOr:
+case ocXor:
case ocNot:
case ocTrue:
case ocFalse:
@@ -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)))
@@ -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!
@@ -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 )
@@ -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)))
diff --git a/formula/source/core/api/token.cxx b/formula/source/core/api/token.cxx
index f426a31..fac4cbd 100644
--- a/formula/source/core/api/token.cxx
+++ b/formula/source/core/api/token.cxx
@@ -90,7 +90,7 @@ bool FormulaToken::IsFunction() const
|| (
RE: [PATCH]fdo 50488 added calc formula XOR as defined in ODFF1.2
Hi Eike, > Argh, I missed to reply to you in the bug that XOR is also in the said CWS we > still hope to be able to integrate back from OOo times. So you duplicated > some work, my bad. Anyway, if it's ready we'll integrate it, adding some > pieces for Excel import/export. However, you attached a patch for DATEDIF > instead of XOR ;-) How stupid of me to attach the wrong patch! I will (re)submit the proper patch file later this afternoon (It is on my computer at home). Should my work not be as good as the code you are hoping to integrate, just disregard my patch. High quality Libreoffice goes first. > I'll browse through the list you provided, maybe there are some things to > remark. Yes, please do. I will wait for your remarks. Winfried ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [PATCH]fdo 50488 added calc formula XOR as defined in ODFF1.2
Hi Winfried, On Wednesday, 2012-06-06 16:35:52 +0200, Winfried Donkers wrote: > Attached patch adds formula XOR to calc. > Possibly the function ought to be included in one or more arrays of functions > that are or are not known in Excel, Lotus, Qpro. Argh, I missed to reply to you in the bug that XOR is also in the said CWS we still hope to be able to integrate back from OOo times. So you duplicated some work, my bad. Anyway, if it's ready we'll integrate it, adding some pieces for Excel import/export. However, you attached a patch for DATEDIF instead of XOR ;-) > In the mean time I will pick another ODFF1.2 function still missing in calc. I'll browse through the list you provided, maybe there are some things to remark. Thanks Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD pgp5YSgXx4SWI.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
