185797 - (xslt_compile) Tracker for stylesheet compilation. TX_COMPILE_BRANCH

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

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

This should allow for much faster execution, stability, interuptability and
forwards-compatible-parsing.

Comment 1

[Axel Hecht]

I just had a peek at 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/transformiix/source/xslt/txInstructions.cpp&rev=1.1.2.2&root=/cvsroot
and I don't like aEs.pushString, pushInt and so.
Please make the state a collection of information, with method names indicating
the purpose of the method. Not a collection of raw data that might or might not
regain it's original meaning.

Comment 2

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

Not sure what you mean.. The execution-state will work as a general-purpose
stack where you can push a bunch of datatypes that the instructions can use for
whatever they want. What else do you want to call them?

The reason there there are different pushes for different datatypes instead of a
generel-purpose push(void*) is so that in case of error the execution-state can
do proper cleanup.

Comment 3

[Axel Hecht]

I hate "general purpose" stack. That's junk data, and pretty useless (or even
confusing) for debugging. Stuff like the qname is state information (if it is, 
I'm not convinced), which should be stored as that. I don't want to delve into
each call into the state to find out what data should be what.
Please do some real analysis on what information belongs into the state, and which
information belongs into the action, and keep that information accessible in the 
state. If you need a stack of xsl:element QNames, then make a stack of that. Don't
mix it with other data. 

(We should decide whether giving the QName for the endtag to the handler is real 
information, DEBUG only information or no information at all.)

Comment 4

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

I don't see that having explicit calls for each "thing" we push is going to 
help. Why have pushNamespace(String), pushLocalName(String) and
pushStringHandlerResult(String) that all do the exact same thing?

Yes, things will be more complex then in the current code, but i don't see how 
we can do interuptability and still have things as safe and clean as they are 
now.

Comment 5

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

on the end-tag note: I would like to remove the namespace and nodename arguments
from the endElement call so that </LRE> and </xsl:element> only needs to call
|aEs.mResultHandler->endElement()|. Module-endElement doesn't need the that
information at all and standalone could keep a stack in the output-handler

Comment 6

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

err... forgot to say (still on the end-tag issue), that i want to do the
result-handler change as a separate patch. This could probably be done together
with atomizing the result-handler.

Comment 7

[Axel Hecht]

when porting xsl:message over to instructions, please drop the special handling
for standalone. Both module and standalone can and should use the console service.
standalone drivers may choose to register a nsIConsoleListener.
Getting the console service could be part of the startup routines, too, having
to get a service for each run may turn out bad.

Comment 8

[Axel Hecht]

virtual type txToplevelItem::getType() = 0;
shouldn't be virtual, but txToplevelItem should hold a member (static?) and
an inline non-virtual should just return that.
This is one of the classic examples alecf ranted about, IIRC. A virtual function
that doesn't really need to be virtual.
Not sure if subclasses can overload the value of a static member var of its
base class.

Comment 9

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

putting a member in there would increase the size of all toplevel items so i
would rather keep it as is. Putting a _static_ member in there would make all
objects have the same type.

Comment 10

[Axel Hecht]

I'd hate to see TxObject survive in all this new code.
On first glimpse, you don't use old tx datastructures no more, so TxObject 
should die.

txStylesheetCompilerState shouldn't hold the parse methods. As far as 
parsing expressions and patterns is concerned, it's the context, not the parser.
They're no-brainers anyway.

I'd say that our macros begin with TX, not TRANSFRMX. Even more for header
files that don't have transfrmx in their name :-).

Comment 11

[Axel Hecht]

txExecutionState::[push|pop][Int|String] should merge into pushQName and popQName.
That's there only use, so do that. Working on txExpandedName. Or, as the handler
ain't atomized yet, on something else.
That function should have a /* deprecated */, too, as the endElement handler 
should someday not require the element name anymore.

there should be more NS_STATIC_CASTs instead of simple C casts.

Comment 12

[Alec Flett]

hey, changes to xpcom have to go through dougt, and I'm not sure he would have
been pleased with the nsError.h stuff - we're not in the habit of putting new
modules into XPCOM...

Comment 13

[Peter Van der Beken [:peterv]]

Alec: so how would one go about having module-specific errors then? Imo we even
want our own JS exceptions for these someday, in which case we need a XSLT XPCOM
error module anyway.

Comment 14

[Alec Flett]

the mechanism just isn't really pluggable right now. I guess I'm just applying
the what-if-everyone-... principle. not every module should be notifying XPCOM
of its existance.

Comment 15

[Peter Van der Beken [:peterv]]

Well, it does seem like this is the only way until XPCOM actually hands us a way
to do this dynamically. The don't-blame-everyone-for-the-shortcomings-of-XPCOM
principle ;-).

Comment 16

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

And it's not likly that we'll run out of errormodules anytime soon. The forumla
allows for 32722 errormodules, which given the current rate of 27 in 5 years
will make us run out in year 6059 :-)

Comment 17

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

sorry, make that year 8057 :-)

Comment 18

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

Attachment: Final patch

This includes everything except the change to nsError.h. Had to zip it since
bugzilla didn't like a 647kB attachment :-)

Comment 19

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

patch landed with the following modifications:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=sicking%25bigfoot.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=03%2F25%2F2003+17%3A32&maxdate=03%2F25%2F2003+19%3A18&cvsroot=%2Fcvsroot

Comment 20

[Peter Van der Beken [:peterv]]

This fix:

nsAString* base = baseURISet ? &baseURI : &mBaseURI;

should have been

const nsAString& = baseURISet ? baseURI : mBaseURI;

IMO.

Comment 21

[timeless]

i don't believe that compiles xp

Comment 22

[Peter Van der Beken [:peterv]]

Then add some casts until it compiles. *Sigh*.

Comment 23

[timeless]

it's not a casting problem.

Comment 24

[Axel Hecht]

let's live with the nsAString* and close this out.

Comment 25

[Axel Hecht]

mass verifying