The other functions wants sweet sweet lovin' too

VERIFIED FIXED in mozilla0.9.9

Status

()

Core
XSLT
P3
normal
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

Trunk
mozilla0.9.9
Other
Other
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
*** Bug 104789 has been marked as a duplicate of this bug. ***
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!
> >+        }
> >         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.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.6

Comment 6

17 years ago
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.
damn! i forgot the modelines, i've added them locally though
added proper errorreports (and assertions) to FunctionCall::evaluateToXXX, 
and 'a' prefixed NumberFunctionCall::evaluate

Comment 10

17 years ago
Comment on attachment 55578 [details] [diff] [review]
ver 3

this patch is obsoleted by petervs
atoms check in
Attachment #55578 - Attachment is obsolete: true

Updated

17 years ago
Attachment #54994 - Attachment is obsolete: true

Updated

17 years ago
Attachment #53517 - Attachment is obsolete: true
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
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Created attachment 57590 [details] [diff] [review]
sync with tip
Attachment #56362 - Attachment is obsolete: true
pushing.. still waiting for review on these
Target Milestone: mozilla0.9.7 → mozilla0.9.8
pushing
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Created attachment 66458 [details] [diff] [review]
sync with tip
Attachment #57590 - Attachment is obsolete: true

Comment 17

16 years ago
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=axel@pike.org
Attachment #66458 - Flags: review+
Created attachment 66642 [details] [diff] [review]
With Pikes commens
Attachment #66458 - Attachment is obsolete: true

Comment 19

16 years ago
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.

Updated

16 years ago
Attachment #66642 - Flags: superreview+
checked in, thanks for reviews
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 21

16 years ago
we didn't verify for a long time.
I really checked, so VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.