[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.

In the mean time I will pick another ODFF1.2 function still missing in calc.

Winfried
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

>> 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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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.

--
[hidden email]  <><, Pseudo Engineer, itinerant idiot

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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.

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.
And there.
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);

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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

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

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice