156464 - Remove static strings from Transformiix

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Peter Van der Beken [:peterv]]

We should get rid of most if not all static strings in Transformiix, most of
them can be replaced by using atoms anyway.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Convert some functions to atoms.

Comment 2

[Axel Hecht]

Attachment: v2

merged to trunk, and added rv checks on qname.init.
If that's ok with peterv, I call this a r=[e-mail]

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 91100 [details] [diff] [review]
v2

record r=Pike.

Comment 4

[Peter Van der Beken [:peterv]]

Note: this bug will be fixed in several stages. I'll start by removing as much
usage of the static strings as possible and then remove all the unused strings
in a last patch.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 91100 [details] [diff] [review]
v2

+#define CHECK_ELEMENT(_name) qname.mLocalName == txXSLTAtoms::##_name
+

Eek, I'd much rather see you pass qname as an argument to this macro, I hate
macro's that make assumptions about random locals being available where they're
used. Any reason to not make this a static inline function in stead of a macro?
Or even better, get the local name atom into a local variable and just use ==
in the if checks below, wouldn't that be the fastest and cleanest thing here?

sr=jst either way.

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Jst happy.

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v2.2

Oops, wrong patch. Pike happy, Jst happy.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Second round v1

Comment 9

[Axel Hecht]

Comment on attachment 92405 [details] [diff] [review]
Second round v1

Looks fine to me, could you change the comment on FunctionCall::toString to
reflect that it just outputs the parameters?

Comment 10

[Axel Hecht]

adding the attachement number to the whiteboard, I'll be confused otherwise

Comment 11

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

Comment on attachment 92405 [details] [diff] [review]
Second round v1

>+void BooleanFunctionCall::toString(String& aDest)
>+{
>+    txAtom* functionName;
>+    switch (mType) {
>+        case TX_BOOLEAN:
>+            functionName = txXPathAtoms::boolean;
>+            break;
>+        case TX_LANG:
>+            functionName = txXPathAtoms::lang;
>+            break;
>+        case TX_NOT:
>+            functionName = txXPathAtoms::_not;
>+            break;
>+        case TX_TRUE:
>+            functionName = txXPathAtoms::_true;
>+            break;
>+        default:
>+            functionName = txXPathAtoms::_false;
>+            break;
>+    }
>+    if (TX_GET_ATOM_STRING(functionName, aDest)) {
>+        FunctionCall::toString(aDest);
>+    }
>+}

Another way to do this would be to have a private virtual function in
FunctionCall called 'getNameAtom' or some such that all FunctionCall inheriters
can override. Then FunctionCall::toString could call that and do the
atom->string conversion so that we only do that once. Not a requirement, but it
would IMHO be nice.

Also, TX_GET_ATOM_STRING will set aDest, not append to it.

>+// XXX This is ugly, but this is the last file to use them,
>+//     once we convert the parser to directly compare with
>+//     atoms we should remove these.

I take it you want to wait with that until next patch. IMHO we don't have to
wait until the lexer is atomized (and i know that Pike has some reservations
wrt that) until we do this.

>+void FunctionCall::toString(String& aDest)
> {
>-    dest.append(this->name);
>-    dest.append('(');
>-    //-- add parameters
>+    aDest.append('(');
>     txListIterator iter(&params);
>-    int argc = 0;
>+    MBool addColon = MB_FALSE;
>     while (iter.hasNext()) {
>-        if (argc > 0)
>-            dest.append(',');
>+        if (addColon) {
>+            aDest.append(',');
>+        }
>+        else {
>+            addColon = MB_TRUE;
>+        }
>         Expr* expr = (Expr*)iter.next();
>-        expr->toString(dest);
>-        ++argc;
>+        expr->toString(aDest);
>     }
>-    dest.append(')');
>-} //-- toString
>-
>+    aDest.append(')');
>+}

It seems that this will only add a ',' after every other expression, instead of
ever one except the first? Also, wouldn't 'addComma' be a better name then
'addColon'?

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: Second round v2

Comment 13

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

Comment on attachment 92446 [details] [diff] [review]
Second round v2

>+txAtom* BooleanFunctionCall::getNameAtom()
>+{
>+    switch (mType) {
>+        case TX_BOOLEAN:
>+            return txXPathAtoms::boolean;
>+        case TX_LANG:
>+            return txXPathAtoms::lang;
>+        case TX_NOT:
>+            return txXPathAtoms::_not;
>+        case TX_TRUE:
>+            return txXPathAtoms::_true;
>+        default:
>+            return txXPathAtoms::_false;
>+    }
>+}

It is kind'a ugly to return a non-addreffed atom, but if you're fine with it
then so am I. I'm also pondering if the above might cause a 'no value returned'
warning on some compilers?

If it doesn't, r=sicking

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: Second round v2.1

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 92553 [details] [diff] [review]
Second round v2.1

Carrying over r=sicking.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 92553 [details] [diff] [review]
Second round v2.1

- In BooleanFunctionCall::getNameAtom(txAtom** aAtom):
+{
+    switch (mType) {
...
+	 default:
+	 {
+	     return NS_ERROR_FAILURE;
+	 }

You should null out the out param before returning here. Same thing in
NodeSetFunctionCall::getNameAtom(), NumberFunctionCall::getNameAtom(), and
StringFunctionCall::getNameAtom().

sr=jst

Comment 17

[Peter Van der Beken [:peterv]]

Landed the TX_BRIDGE_1_1_BRANCH to the trunk.

Comment 18

[Axel Hecht]

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