advice needed: bug44456 addition of ODF-formula datedif to calc

[Winfried Donkers]

Attached diff is NOT intended to be pushed at this state.

>> I have added the formula 'datedif', as defined in ODF 1.2 to calc (see diff).
>
> Hey, great!
>
>> 2. the calculation in DateDif(..) uses mean values for days in year
>> (365.2425) and for days in month (30.4369). That will in some
>> instances lead to -seemingly?- incorrect results. Should I do
>> something about that, i.e make the code date-concious?
>
> Yes please, make it leap year aware and use correct values for days in
> months.

I have done so now.

>> 3. The various defined values for paramter fmt (d, m, y, yd, ym, md)
>> are 'hard' in the code ("d", "m", "y", "yd", "ym", "md"). This does
>> not look neat to me. is there a preferred way of doing such things?
>
> No, hard coded is fine in this case as the values are not to be
> translated. However, string comparison should be case insensitive to
> allow also upper case values. IMHO Excel does that. Btw, do you have
> access to an Excel version to compare?

>I have a German Excel 2010. Excel allows upper case values. The code is
>not translated. In German Excel 2010 it is still "y" and not "j".

I have made the code case-insensitive for the format parameter.

>> 5. ODF 1.2 defines DATEDIF, but the languages I've seen using this
>> function use DATEDIFF. Why is ODF different (or isn't it)?
>
> The function originates from Lotus 1-2-3 where it was called DATEDIF,
> MS-Excel called it the same, and ODF just adopted that name. The
> DATEDIFF function you mention is something different (VBA?).

I don't know if the function needs to be added to xlformula.cxx and/or lotform.cxx (see comment in code)

> Now some nitpicks on your code ;)

Please nitpick; it improves my patches :)

>> +++ b/sc/source/core/tool/addinhelpid.cxx
>
> Is there a specific reason why you implemented this in the
> Analysis-AddIn? I don't recall exactly whether this function was
> provided in Excel as an AddIn or built-in. If as AddIn it makes sense to
> implement it in our AddIn for import/export from/to Excel, if not then
> a built-in function may be easier.

>Excel 2010 has only some special things in the now called "Analysis
>ToolPak", e.g. ANOVA. Details see [1]. Functions like BESSELJ are now
>integrated into the normal set of functions. DATEDIF is available
>without that "Analysis ToolPak". DATEDIF is still not listed in the
>function wizard, but you have to write it directly.

Originally I put the code in analysishelper.cxx because I used function yearfrac as a guide. At that time I was not aware of standard and addin-functions.
Now I have put the code in interpr2.cxx, as Regina suggested.
The function is listed in the function wizard.
I do not have Excel, so I can't test if an xls-file with DATEDIF() imports ok.


There is one problem with building: sc/qa/unit/ucalc.cxx produces an error:
  ucalc.cxx:3413:Assertion
  Test name: N12_GLOBAL__N_14TestE::testFunctionLists
  assertion failed
  - Expression: pFunc->getFunctionName().equalsAscii(aTests[i].Functions[j])
  - Unexpected function name
I don't know how to correct this. I added the function to formula/inc/formula/compiler.hrc and incremented SC_OPCODE_LAST_OPCODE_ID.
That' s why I commented out some lines in ucalc.cxx for the time being.

The method used to calculate the difference in days when years (or years and months) are to be ignored results in a negative result for datedif(20-11-2010, 15-10-2012, "yd"). I think that is in accordance with the definition in ODF1.2, but it does look strange at first sight.

Any comment (including nitpicks :)) is welcome. When the open questions (regarding xlformula.cxx, lotform.cxx and ucalc.ccx) have been addressed, I intend to submit the patch officially.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: fdo44456-20120505.diff
Type: text/x-patch
Size: 13511 bytes
Desc: fdo44456-20120505.diff
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20120505/16c3b6d2/attachment.bin>