Bug 185797 (xslt_compile)

Tracker for stylesheet compilation. TX_COMPILE_BRANCH

VERIFIED FIXED

Status

()

Core
XSLT
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: sicking, Assigned: peterv)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

Comment 1

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

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

15 years ago
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

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

15 years ago
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

15 years ago
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

15 years ago
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...
(Assignee)

Comment 13

15 years ago
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

15 years ago
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.
(Assignee)

Comment 15

15 years ago
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 ;-).
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 :-)
sorry, make that year 8057 :-)
Created attachment 118509 [details]
Final patch

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

Comment 20

15 years ago
This fix:

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

should have been

const nsAString& = baseURISet ? baseURI : mBaseURI;

IMO.

Comment 21

15 years ago
i don't believe that compiles xp
(Assignee)

Comment 22

15 years ago
Then add some casts until it compiles. *Sigh*.
No longer depends on: 104346

Comment 23

15 years ago
it's not a casting problem.

Comment 24

15 years ago
let's live with the nsAString* and close this out.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 25

15 years ago
mass verifying
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.