104758 - The other functions wants sweet sweet lovin' too

Status: VERIFIED FIXED

(Core :: XSLT, defect, P3)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

BooleanFunctionCall and StringFunctionCall wants love too. Especially 
BooleanFunctionCall is buggy, 3 out of 4 functions does the wrong thing. 

StringFunctionCall mostly works, only substring() has a few bugs (The usual NaN 
and +-Inf problems), so it's doubtfull if the compleate revamp is worth the 
trouble. Let me know if you want me to patch the old code instead of rewring.

I've also made FunctionCall::evaluateAs(String|Number) a bit more stable, as 
well as implement FunctionCall::evaluateAsBoolean for compleatness sake.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: ver 1

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*** Bug 104789 has been marked as a duplicate of this bug. ***

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 53517 [details] [diff] [review]
ver 1

>+            String arg1, lang;
>+            evaluateToString((Expr*)iter.next(),context, cs, arg1);
Space before context.

>+            Node* node = context;
>+            while (node) {
>+                if (node->getNodeType() == Node::ELEMENT_NODE) {
>+                    Attr* attr = 
>+                        ((Element*)node)->getAttributeNode(XML_LANG_ATTR);
>+
>+                    if (attr) {
>+                        lang = attr->getNodeValue();
>+                        break;
>+                    }
>+                }
>             }
>-            break;
>+            arg1.toUpperCase(); // case-insensitive comparison
>+            lang.toUpperCase();
>+            MBool result = lang.indexOf(arg1) == 0 &&
>+                           (arg1.length() == lang.length() ||
>+                            arg1.charAt(lang.length()) == '-');
>+            return new BooleanResult(result);
Could you do an early return if lang is not found?
And move the arg1 and evaluateToString to after the early return.
You might even be able the use the sexy new getAttr.

>+        }
>         default:
>-            break;
>+        {
>+            if (!requireParams(0, 0, cs))
>+                return new BooleanResult(MB_FALSE);
This seems unnecesary?
>+
>+            return new BooleanResult(MB_FALSE);
This too?
>+        }
>     }
>-    delete iter;
>-    return new BooleanResult(result);
>+    return new BooleanResult(MB_FALSE);

>+    ExprResult* exprResult = expr->evaluate(context, cs);
>+    if (!exprResult)
>
>+        return Double::NaN;
No empty line between the if and the return.

>+            String resultStr;
>+            while(iter.hasNext()) {
>+                evaluateToString((Expr*)iter.next(),context, cs, resultStr);
Space before context.

>+            evaluateToString((Expr*)iter.next(),context, cs, arg1);
>+            evaluateToString((Expr*)iter.next(),context, cs, arg2);
Same.

>+            String resultStr;
>+            if (iter.hasNext())
>+                evaluateToString((Expr*)iter.next(),context, cs, resultStr);
Same.

>+                if (c == ' ' ||
>+                    c == '\n' ||
>+                    c == '\t' ||
>+                    c=='\r') {
Spaces ;)

>+                    addSpace = MB_TRUE;
>                 }
>+                else {
>+                    if (addSpace && !first)
>+                        normed.append(' ');
>+
>+                    normed.append(c);
>+                    addSpace = MB_FALSE;
>+                    first = MB_FALSE;
>                 }
>             }
>+            return new StringResult(normed);
Hum, this leaves a trailing space, no?

>+        case STARTS_WITH:
>+        {

>+            return new BooleanResult(arg1.indexOf(arg2) == 0);
Would it be faster to just compare with substring of arg 1 with
length of arg2? Not sure with our strings :(.

>+            start = floor(start + 0.5);
I haven't read IEEE 754 so i'll trust you on this ;).

>+            evaluateToString((Expr*)iter.next(),context, cs, arg1);
Just do a search on ",context" and replace it with ", context".

>+                arg1.subString(idx+len, arg2);
Spaces around the +.

>+            if (idx >= 0) {
>+                PRInt32 len = arg2.length();
>+                arg2.clear();
>+                arg1.subString(0, idx, arg2);
>+                return new StringResult(arg2);
>             }
I think you don't need len.

It'd be nice if you could change the parameters to the functions you change to
*a*Parameter. Thanks for doing this!

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: ver 2

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> >+        }
> >         default:
> >-            break;
> >+        {
> >+            if (!requireParams(0, 0, cs))
> >+                return new BooleanResult(MB_FALSE);
> This seems unnecesary?
> >+
> >+            return new BooleanResult(MB_FALSE);
> This too?

Changed both to return an error (which should be replaced with 0 eventually)

> >+                    addSpace = MB_TRUE;
> >                 }
> >+                else {
> >+                    if (addSpace && !first)
> >+                        normed.append(' ');
> >+
> >+                    normed.append(c);
> >+                    addSpace = MB_FALSE;
> >+                    first = MB_FALSE;
> >                 }
> >             }
> >+            return new StringResult(normed);
> Hum, this leaves a trailing space, no?

It shouldn't since i only add the space once i find non-space char, so the last 
|addSpace| will never be used.

> >+            return new BooleanResult(arg1.indexOf(arg2) == 0);
> Would it be faster to just compare with substring of arg 1 with
> length of arg2? Not sure with our strings :(.

Substring requires some memoryallocating, so i think this is faster

> >+            start = floor(start + 0.5);
> I haven't read IEEE 754 so i'll trust you on this ;).

Neither have I :)
but the spec says this should work


Did everything else, also made the code return |StringResult("error")| 
whereever we should return 0. I also did a small fix to format-number, sorry to 
add an unrelated thing, but i discovered it while running the string xalan 
tests.

Comment 6

[Axel Hecht]

well, you know what's coming.
Yes.
Modelines.

But some real stuff too.

BooleanFunctionCall and FunctionCall could use some XXX foo or so do indicate
where to do error reporting. As they don't have "error" in them.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: ver 3

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

damn! i forgot the modelines, i've added them locally though

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

added proper errorreports (and assertions) to FunctionCall::evaluateToXXX, 
and 'a' prefixed NumberFunctionCall::evaluate

Comment 10

[Axel Hecht]

Comment on attachment 55578 [details] [diff] [review]
ver 3

this patch is obsoleted by petervs
atoms check in

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: ver 4

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

synced with petervs atomtables, basically removing the temporary atom 
creation/release and use the static txXMLAtoms::lang.

Pushing this since there is other stuff that is more important stuff targeted 
for 0.9.6

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: sync with tip

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

pushing.. still waiting for review on these

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

pushing

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: sync with tip

Comment 17

[Axel Hecht]

Comment on attachment 66458 [details] [diff] [review]
sync with tip

>Index: source/xpath/BooleanFunctionCall.cpp
>@@ -32,9 +33,10 @@
> /**
>  * Creates a default BooleanFunctionCall, which always evaluates to False
> **/
>-BooleanFunctionCall::BooleanFunctionCall(short type) : FunctionCall()
>+BooleanFunctionCall::BooleanFunctionCall(BooleanFunctions aType)
> {
>-    switch ( type ) {
>+    mType = aType;
>+    switch (aType) {
>         case TX_BOOLEAN :
>             name = XPathNames::BOOLEAN_FN;
>             break;

you know me, :mType(aType)

>@@ -61,49 +62,76 @@
...
>+                    if (elem->getAttr(txXMLAtoms::lang, kNameSpaceID_XML, lang))
cut that into two lines, just a bit too long.
Nice to see lang() working finally.

>Index: source/xpath/Expr.h
You spend quite a few aFoo in the arguments, but not in the header.
Do so for the FunctionCall object here.

>Index: source/xpath/FunctionCall.cpp
These are internal functions, don't be too paranoid. We should have
caught those errors somewhere else already. So just assert, with a comment.
(Taking this from a jst sr nit)


>Index: source/xpath/StringFunctionCall.cpp

>+StringFunctionCall::StringFunctionCall(StringFunctions aType)
>+{
>+    mType = aType;
guess what? yep, :mType(aType) :-)

with those, r=[e-mail]

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: With Pikes commens

Comment 19

[jag (Peter Annema)]

sicking: it's "its" when you mean the possessive (yes, I know you were simply
doing a c&p + modify, but ...). And yes, I'm picking nits :-)

sr=jag with that changed.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in, thanks for reviews

Comment 21

[Axel Hecht]

we didn't verify for a long time.
I really checked, so VERIFIED.