Date: prev next · Thread: first prev next last
2012 Archives by date, by thread · List index


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

Attachment: pgpQENOTNTQm7.pgp
Description: PGP signature


Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.