230633 - Constify Transformiix code a bit

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Peter Van der Beken [:peterv]]

I've contified some data in Transformiix, I've also moved to the new XPCOM
static atoms. Here's what size shows:

Trunk
Segment __TEXT: 679936
Segment __DATA: 69632
Segment __LINKEDIT: 487424
total 1236992

With constify patch
Segment __TEXT: 679936
Segment __DATA: 61440
Segment __LINKEDIT: 487424
total 1228800

With atoms patch
Segment __TEXT: 663552
Segment __DATA: 65536
Segment __LINKEDIT: 491520
total 1220608

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Constify patch v1

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Static atoms patch v1

Comment 3

[Peter Van der Beken [:peterv]]

Actually, nevermind the constify patch v1. It doesn't work, I'll attach a new
one. Here's the numbers with atoms and the new constify patch:

Segment __TEXT: 659456
Segment __DATA: 65536
Segment __LINKEDIT: 491520
total 1216512

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Constify patch v1.1

Comment 5

[Axel Hecht]

Comment on attachment 138808 [details] [diff] [review]
Static atoms patch v1

AFAICT,
+static const nsStaticAtom XMLAtoms_info[] = {
+#define TX_ATOM(name_, value_) { value_, &txXMLAtoms::name_ },
+XML_ATOMS
+#undef TX_ATOM
+};
leaves us with a 
{
{foo, &txXSLTAtoms::foo},
{bar, &txXSLTAtoms::bar},
}

Note the trailing , in the last entry, does that compile cross platform? IIRC
it does not for enums or so.

Comment 6

[Peter Van der Beken [:peterv]]

I copied that from the layout atoms (see for example nsHTMLAtoms:
http://lxr.mozilla.org/seamonkey/source/content/shared/src/nsHTMLAtoms.cpp#47),
and it certainly seems to be compiling there :-).

Comment 7

[Axel Hecht]

Comment on attachment 138808 [details] [diff] [review]
Static atoms patch v1

>Index: source/base/txStringUtils.cpp

>+PRBool
>+TX_StringEqualsAtom(const nsASingleFragmentString& aString, nsIAtom* aAtom)
>+{
>+    const char* atom;
>+    aAtom->GetUTF8String(&atom);
>+
>+    PRUint32 ASCIILength = strlen(atom);
>+    nsDependentCString ASCIIString(atom, ASCIILength);

This string ain't needed, drop ASCIIIter and just use atom.
Or rename it atomchar or so.

>+
>+    PRUint32 UTF16Length = aString.Length();
>+    if (ASCIILength != UTF16Length) {
>+        return PR_FALSE;
>+    }
>+
>+    const char* ASCIIIter;
>+    ASCIIString.BeginReading(ASCIIIter);
>+
>+    const PRUnichar* UTF16Iter;
>+    aString.BeginReading(UTF16Iter);
>+
>+    while (ASCIIIter) {
>+        if (PRUnichar(*ASCIIIter) != *UTF16Iter) {
>+            return PR_FALSE;
>+        }
>+        ++ASCIIIter;
>+        ++UTF16Iter;
>+    }
>+
>+    return PR_TRUE;
>+}
>+

Comment 8

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

Comment on attachment 138816 [details] [diff] [review]
Constify patch v1.1

>@@ -374,7 +374,8 @@ txStylesheetCompiler::endElement()
>         }
>     }
> 
>-    txElementHandler* handler = NS_STATIC_CAST(txElementHandler*, popPtr());
>+    const txElementHandler* handler =
>+        NS_STATIC_CAST(const txElementHandler*, popPtr());
>     rv = (handler->mEndFunction)(*this);
>     NS_ENSURE_SUCCESS(rv, rv);

I'm a little worried that this won't compile cross-platform ( casting from
non-const to const using explicit static-cast). But if it works then this
definitly looks better then any alternatives.

>+#define INIT_HANDLER_WITH_ELEMENT_HANDLER(_name)                     \
>+    INIT_HANDLER(_name);                                             \
>+                                                                     \
>+    rv = gTx##_name##Handler->init(&gTx##_name##ElementHandler, 1);  \
>+    if (NS_FAILED(rv))                                               \
>+        return PR_FALSE
>+
>+#define INIT_HANDLER_WITH_ELEMENT_HANDLERS(_name)                    \
>+    INIT_HANDLER(_name);                                             \
>+                                                                     \
>+    rv = gTx##_name##Handler->init(gTx##_name##ElementHandlers,      \
>+                                   NS_ARRAY_LENGTH(gTx##_name##ElementHandlers)); \
>+    if (NS_FAILED(rv))                                               \
>+        return PR_FALSE

It think it'd fine to not differentiate between handlers that have 1 and
handlers that have 2+ element-handlers. Just creating a list with one element
shouldn't change the code.

r=me either way

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 138808 [details] [diff] [review]
Static atoms patch v1

TX_StringEqualsAtom():

+    nsDependentCString ASCIIString(atom, ASCIILength);

Like Pike said, loose ASCIIString.

sr=jst

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 138816 [details] [diff] [review]
Constify patch v1.1

Doh.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 138816 [details] [diff] [review]
Constify patch v1.1

sr=jst

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: Patch that was checked in

Comment 13

[Peter Van der Beken [:peterv]]

Visual C++ seems to have a bug that causes it to break on

struct whatever {
    const int i;
};

const whatever temp = { 0 };

which is valid afaik, so i had to remove a bunch of const's from the patch.
Fortunately it didn't seem to have an influence on the decrease in the Z
numbers. Decrease in codesighs was about -11k on Windows, -17k on Linux and -22k
on OS X.

Comment 14

[Peter Van der Beken [:peterv]]

Fixed.

Comment 15

[Axel Hecht]

Someone left a pile of poo in the living room. And I reviewed it. Darn.

TX_StringEqualsAtom regressed, patch coming up.

Comment 16

[Axel Hecht]

Attachment: check the char instead of the char* for null termination

Comment 17

[Axel Hecht]

Comment on attachment 139413 [details] [diff] [review]
check the char instead of the char* for null termination

requesting r/sr from peterv for one-line-bustage-fix

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 139413 [details] [diff] [review]
check the char instead of the char* for null termination

sure

Comment 19

[Axel Hecht]

attachement 139413 checked in