Closed Bug 185797 (xslt_compile) Opened 22 years ago Closed 21 years ago

Tracker for stylesheet compilation. TX_COMPILE_BRANCH

Categories

(Core :: XSLT, defect)

Product:
Core
Component:
XSLT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: sicking, Assigned: peterv)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Final patch
105.94 KB, application/x-zip-compressed
Details 
This should allow for much faster execution, stability, interuptability and
forwards-compatible-parsing.
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.
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.
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.
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.
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 :-).
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.
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...
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.
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.
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 :-)
Attached file Final patch — 
This includes everything except the change to nsError.h. Had to zip it since
bugzilla didn't like a 647kB attachment :-)
This fix:

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

should have been

const nsAString& = baseURISet ? baseURI : mBaseURI;

IMO.
i don't believe that compiles xp
Then add some casts until it compiles. *Sigh*.
it's not a casting problem.
let's live with the nsAString* and close this out.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
mass verifying
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: