Closed Bug 117658 (rewrite_variable) Opened 23 years ago Closed 21 years ago

rewrite variable handling

Categories

(Core :: XSLT, defect, P5)

Product:
Core
Component:
XSLT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Future

People

(Reporter: sicking, Assigned: sicking)

References

Details

(Whiteboard: TX_BRIDGE_1_1_FIXED)

Attachments

(9 obsolete files) 

I've started to rewrite our variable code. This should take care of most (all?) 
current variables bugs:

 * make sure only the right variables are in scope when instancing templates
 * key them on expanded name instead of qname
 * import precedence
 * global-variable dependencies

should fix bug 92929 and take care of the variable part of bug 96802 and bug 
83651
oh, and don't worry about merge conflicts, this shouldn't be done before 
neither the new output or context
Blocks: 83651, 92929, qname
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.9
Blocks: 73492
Attached patch patch to fix, ver 1 (obsolete) — — Splinter Review 
ok, this is it.. Our current IsValidQName is pretty broken, so I moved
xpath/ExprLexerChars.cpp to xml/XMLUtilsChars.cpp and used that. The file isn't
changed at all except that the classname is changed
I guess I should explain a bit what i've done. To take care of bug 92929 I've 
made the ProcessorState lazy-evaluate global variables. So when a global 
variable is requested it first checks if we know the value of it (i.e. it was a 
specified stylesheet-parameter, or we've evaluated the variable previously). If 
we don't know the value it calls the XSLTProcessor to evaluate the variable.

In other words, when we find a global xsl:variable/param we do no evaluation, 
but instead just add the element to the ProcessorState. Then the first time the 
variable is requested we evaluate it.
oh, and i replaced the VariableSetStack with a linked list of txVariableMaps. 
All txVariableMaps are created on the stack and contains a pointer to 
their "parent" map.

To get a variable I just ask the topmost map, which then recursivly calls the 
parents. This stack of maps only contains the local variables, the global 
variables have to be manually queried.
Pike rambles in a bug:
I don't like XSLTProcessor being a member of ProcessorState.
Linking forth and back is a design bug, IMHO.

The problem: gobal variables are evaluated without any order.
(http://www.w3.org/TR/xslt#top-level-variables)
And they may reference other global vars, as long as there are no circular
references.
So the global vars and pars have to be evaluated somewhat lazily. There are two
obvious points where this can happen.
1) on variable reference (this is what Jonas' patch does)
2) at the end of the top-most processTopLevel

Issues for 1):
ProcessorState needs to do processVariable. There seem to be three ways to do
this:
a) make the XSLTProcessor a member of ProcessorState. (I strongly oppose to
  that)
b) make all processFoo methods in XSLTProcessor static, and XSLTProcessor truly
  stateless. (Sounds like a reasonable idea in general, but this means killing
  XSLType, too, for example, so quite a big patch)
c) create a new XSLTProcessor with a 0 output handler. (This will cause some
  load for creating the processor, but we don't have that many global vars, and
  we only do it once for each var. Fair enough, IMHO)

Issues for 2)
to catch circular references in variable expressions, we have to detect if the
evaluation of an expression failed for 'undefined var' or any other reason. 
That means changing the signature of evaluate to return a nsresult.
(Another good thing in general, but now?)

Others? (sounds like I vote for 1c, huh?)

I'm not too happy about the unstacking of the variable binding maps. We have 
dropped a bunch of stacks, but this one is different. The variable bindings
do depend on each other, and the order in which they are set in the 
ProcessorState does not necessarily impose the order of the maps. I'd prefer
a ProcessorState::pushVariableMap(&aMap) and 
ProcessorState::discardVariableMap() and make pushVariableMap wire the maps
up. I like having the linked list right in the maps, though.
pushing
Target Milestone: mozilla0.9.9 → mozilla1.0
I don't see it as a problem having ProcessorState call XSLTProcessor. The only
thing we have to watch out for is that XSLTProcessor doesn't have state which
changes during execution of a stylesheet.

The only state that does that is mAttrSetsStack and mResultHandler. I removed
mAttrSetsStack since that could cause problem, but I didn't remove
mResultHandler since for the current call it will not be used (it's immidealy
replaced with an RTF handler). I do think it should be moved to the
ProcessorState, together with mOutputHandler, but i'd rather do that after 1.0.

What I don't like is that we reuse the same ProcessorState, which has the state
of doing something else.

I'd like to create a new ProcessorState and use that. Then we would have the
same state and use the same processor as if we would have processed the variable
before starting the execution, so there's no reason it shouldn't work.

However I currently can't do that since creating a new ProcessorState and
setting it up would be a major undertaking. The way to fix this is to split up
ProcessorState into txStyleSheet and txProcessState (or txExecutionState). 

txStyleSheet would handle the "parsed" stylesheet; keys, templates,
attribute-sets etc, i.e. the result of processTopLevel.

txProcessState would handle the current state during execution; current template
rule, current variable-stack, current nodeset etc.
(sorry, i stopped midair)

Then we very cheaply create a new txProcessState that reffered to the same
txStyleSheet. So we would be guaranteed to have the exact same state as if the
variable would have been processed before we started the execution.
If at all possible I'd like to see

b) make all processFoo methods in XSLTProcessor static, and XSLTProcessor truly
  stateless. (Sounds like a reasonable idea in general, but this means killing
  XSLType, too, for example, so quite a big patch)

I don't like the mutual members either. I do like the ProcessorState splitup
that Jonas proposes (he should file a bug on that ;-)). However, even with that
ProcessorState splitup, I still wouldn't like mutual members.
but we don't have mutatial members, the state isn't a member of the processor. 
And IMO it shouldn't be for several reasons, like making parsed stylesheets 
harder and making the XSLTProcessor much more state-full.

I agree that making the processXXX methods static has the neatness of 
bulletproofing our code, however I think it will add some complexity since 
we'll need to handle the little state that the processor *should* have (mostly 
stylesheet parameters i think)

Anyways, I don't see that b) can really be done if this bug should have any 
chance of getting fixed for 1.0. The mergeconflicts with Pikes branch would 
probably be pretty big and I'm not sure that I'd have time to remove 
mOutputHandler, mResultHandler and mHaveDocumentElement from XSLTProcessor.

What would be possible is to do c) since it would codewise be a very small 
change. It would sort of get us closer to b) logic-wise since it requires that 
the processVariable and it's calltree don't use any state in the XSLTProcessor. 
Even though it doesn't enforce it at compiletime like b) does. 

So if that is good enough for you guys then I'm fine with it.
Attached patch version 2 (obsolete) — — Splinter Review 
does option c)

I have not changed the variables-stack-crawling from comment 5 since i'd need 6
functions instead of the current 2:
ProcessorState::(push|pop)LocalVariables	when recursing one more step in

						for example processChildren
ProcessorState::(push|pop)LocalVariablesStack	when entering a template
VariablesMap::(s|g)etParentMap			crawling up and down the stack

Instead of
ProcessorState::(s|g)LocalVariables
Attachment #72327 - Attachment is obsolete: true
I have a few issues, first, Jonas and I agreed that the chars stuff should go
to bug 136272, that should decrease the size of this quite a bit.

I'm not so sure I like the |txVariableMap|s on the stack and creating them
all the time.
How about this, kinda pseudo code
case XSLType::VARIABLE:
    if (!localVars) {
        localVars = createNewVariableMap (this might be done by ProcessorState)
    }
    eval and bind

and before exiting the function
if (localVars)
    destroyVariableMap (maybe in ProcessorState, too)

That should keep the VariableMap list short, and put the work where it should
be.

Another one is ownership in txVariableMap. This looks like quite some piece of
code, I wonder if we shouldn't just ::clone() the expression result at the one
point where we'd need it.

A comment in processTemplate why we're loosing the previous variable bindings
would be good.
> case XSLType::VARIABLE:
>     if (!localVars) {
>         localVars = createNewVariableMap (this might be done byProcessorState)
>     }

When/where would we set localVars to null? And how would we get a local 
variable (for example in an xsl:value-of) if localVars is null?

> Another one is ownership in txVariableMap. This looks like quite some piece of
> code, I wonder if we shouldn't just ::clone() the expression result at the one
> point where we'd need it.

I don't think it's worth the extra cycles, the code is there and it works. 
Could we stick with the current way, the code is there and it works.
err.. sorry, it's late, only ment to say "the code is there and works" once :-)
> and before exiting the function
> if (localVars)
>     destroyVariableMap (maybe in ProcessorState, too)

And what should we set localVars to here?
pushing :(
Target Milestone: mozilla1.0 → mozilla1.1alpha
could you move the char stuff to bug 136272?
I'm not happy about the logic for params, that is, owning vs. non-owning
bindings. I'd prefer to clone() the exprResult, and just have owning variable
bindings.
I don't like at all that we're still poluting the world with txVariableMaps.
Those should just be created once we encounter a xsl:variable, or a xsl:param.
That requires to fix processAction to iterate over siblings, which is out of
the scope of this bug. But it's easy, so I'm tempted to push this bug behind 
instead of filing a "rewrite variable handling" bug once this code lands.
...
Target Milestone: mozilla1.1alpha → mozilla1.1beta
Alias: rewrite_variable
Attached patch sync with tip (obsolete) — — Splinter Review 
this does nothing except sync with tip, still waiting for a review
Attachment #81758 - Attachment is obsolete: true
Comment on attachment 87962 [details] [diff] [review]
sync with tip

txVarialbeMap.* are missing :-(
Attachment #87962 - Flags: needs-work+
ugh, i've forgot to -N the patch. Will attach a new one tonight, but the files
should be the same as in attachment 81758 [details] [diff] [review]
Attached patch with -N (obsolete) — — Splinter Review 
same as attachment 87962 [details] [diff] [review] but diffed with -N
Attachment #87962 - Attachment is obsolete: true
Jonas and I had a little chat, and this is it:
1) creating the varialbe maps only when needed is pushed to another patch and
phase 2, this bug stays open when phase 1 lands (which is gonna grow out of
attachement 88190)

2) we're still not on agreement whether txVariableMap should own all bindings,
or if we should have a magic to just own most of them.
(params that don't use their default values are owned by the variable map
in the caller)
Ownership of ExprResults might die one day, Jonas would like to refcount those.
I really favour using clone for that one case and be done with that. 
txVariableMap just has way to much code, IMHO. Jonas is worried about the cycles
going into the clone though.

My idea right now:
ProcessorState.h could just do a 
class txVariableMap;
and ProcessorState.cpp and XSLTProcessor.cpp include txVariableMap.h, which 
does something like this:

class txVariableMap : private txExpandedNameMap
{
public:
  txVariableMap(txVariableMap* aParentMap) : txExpandedNameMap(MB_TRUE),
    mParentMap(aParentMap)
  {}

  nsresult bindVariable(const txExpandedName& aName, ExprResult* aValue)
  {
    return add(aName, aValue);
  }

  ExprResult* getVariable(const txExpandedName& aName)
  {
    ExprResult* res = (ExprResult*)get(aName);
    if (!res && mParentMap)
      res = mParentMap->getVariable(aName);
    return res;
  }
}
Needs some comments, but that would be it, I think. Not even a .cpp anymore, 
and a good chunk of code reuse.
Keywords: review
looking at variable20, that is regressed intentionally, right? As in, that test
is broken?
yep, I beleve that they are wrong.
first is trunk time, second is speed up Jonas, third is speed up by my patch.

 alphabetize:   26312  5.3%   6.8%
     attsets:   10755  0.8%   4.1%
        avts:   53374  1.1%   1.0%
        axis:    2151 -1.6%  -1.5%
   backwards:   15148  8.5%  10.5%
     bottles:   66436 29.5%  30.7%
     breadth:   82312 12.4%   2.7%
      brutal:   22096 12.1%   8.1%
       chart:   21492  3.4%   1.3%
    creation:   71857 10.0%   7.8%
     current:    4427 11.3%  11.2%
      dbtail:   45207 10.2%  10.7%
       decoy:  862250  8.4%   8.0%
       depth:   20146 14.1%  14.9%
     encrypt:   26443 16.7%  16.4%
   functions:  217134  6.4%   5.7%
        game:    6467  9.6%  -7.4%
        html:    3179  9.9%  -19.4%
        html:    3179  9.9%  11.5% /// second run
    identity:   92299 18.4%  17.5%
   inventory:   12280 10.8%   7.6%
      metric:    7650  8.4%   7.3%
      number:    4637 10.2%   4.1%
 oddtemplate:    2054  1.0%   0.0%
    patterns:  508390  7.8%   4.9%
    patterns:  508390  7.8%   7.9%  /// second run
 prettyprint:  109624 11.3%  11.0%
    priority:    5402  1.2%  -9.2%
    priority:    5402  1.2%   3.6%   /// second run
    products:   30854  4.0%   3.1%
      queens:   80389 36.6%  35.2%
    reverser:   52739 26.1%  23.7%
  stringsort:  103234 14.5%  14.4%
   summarize:   20236  2.5%   3.3%
       total:    3295  3.6%  -6.5%
       tower:  241442 42.1%  41.4%
       trend:  166036  1.2%   1.1%
       union:    1972 10.2%   6.6%
       xpath:    1754 -5.6%   7.5%
   xslbench1:   13592  5.0%  -4.3%
   xslbench2:   54831 11.1%  10.5%
   xslbench3:  112377  6.4%   4.6%
   xslbench3:  112377  6.4%   6.8% /// second run

On four bad draws on my patch I ran the examples in the foreground, those
are short tests, so that's prolly either bad timing or too much load doing
other stuff during that runs. Makes the numbers a bit odd.

I'd say that performance win is comparable, I really don't like to run all
these tests exclusively once more.

12% overall for me, 14 for Jonas, but I'd give nothing on those two %.
Tuning gave 13% for me.
Those numbers aren't fair, XSLTMark UI needs improvement here. Or some analysis
of the detailed numbers. Later.

Comments on Jonas' patch, won't build on mac, comment in XSLTProcessor.h is
wrong. I'll post my patch later.
Changes/comments:
txExpandedNameMap has const arguments, I guess you did that in another patch,
too.

ProcessorState::addGlobalVariable had a ExprResult argument that was never used.
I removed that.

a few null checks, a few rv = NS_OK;, just because I care.

My patch doesn't regress variable20, I won't grok why in this second.
Attached patch alternative patch (obsolete) — — Splinter Review 
oh, I merged the string changes, too.
hmm.. i'm a little surpriced by those numbers. I wouldn't be surpriced if my
version doesn't give a big speedup, but i do find it strange that it would
actually be *slower* then your way on *any* testcase. Unless you have done more
then just do cloning instead of non-owning?

The reason your patch doesn't regress variables20 is probably that you don't
walk up the variable stack when adding a variable to make sure that it doesn't
already exist. (That could be the reason that your patch is faster, but i doubt
that that makes a big difference)
yeah, found my bug, removed recursion to ease inlining, too.
made new numbers, this time for the pattern test only (lack of time/patience)

mypatch:
patterns	4574.36 ms	457436 ms	65.48
patterns	4519.89 ms	451989 ms	26.98
Jonas:
patterns	4497.69 ms	449769 ms	58.42

Jonas is somewhere between .5 and 1.7% faster. I'll attach my new
txVariableMap.h
in a second
Attached file txVariableMap.h (obsolete) — 
no recursion, check the existing parent variable maps for bindVariable
the variable in ProcessorState::addGlobalVariable is for specifying stylesheet
parameters. Look at the comment in XSLTProcessor::processTopLevel
more precise about last line in #26, the wrong comment is in XSLTProcessor.h,
processParameters; it claims to return a NamedMap and other strange things.

re 32: global parameters.
forgot about those, but I would prefer a different aproach:
have two entry points in ProcessorState,
    nsresult addGlobalVariable(Element* aVarElem,
                               ImportFrame* aImportFrame);
and
    nsresult addGlobalParameter(Element* aVarElem,
                                ImportFrame* aImportFrame);

addGlobalParameter would check for a given param in a mParameters map.
If found, it would get the ExprResult, and add that into the mGlobalVars,
otherwise it would call addGlobalVariable.

I prefer this as this keeps the mParameters encapsulated. The processorstate
knows the params, and it decides what to do with a global param.

Note that in the picture of
http://www.mozilla.org/projects/xslt/whitepaper/xslt-states.html,
mParameters is a STATE0 thing, while mGlobalVariableValues is a STATE1 thing.
If indeed the global parameters should live in the same class that has the
::addGlobalVariable (which will be ProcessorState when this is checked in) then
i agree, though it might be better to do.

    nsresult addGlobalVariable(Element* aVarElem,
                               ImportFrame* aImportFrame,
                               MBool aParameter);

since the two functions will be virually identical. Not a biggie though
Attached patch alternative patch v2 (obsolete) — — Splinter Review 
merged with QNames, fixed a regression in txVariableMap (I got only one parent,

bah, that's a while now)
I prefer separate entry points for global params and global vars, so I made 
that.
Merging the QNames was less work than the strings, IIRC. Worst conflict was 
the attribute set recursion.
Attachment #90417 - Attachment is obsolete: true
Attachment #90469 - Attachment is obsolete: true
peace bro'er.

How about this:

make txVariableMap have two txExpandedNameMap members, one for owning, one for
non-owning bindings. That way my two major concerns would be addressed, first,
code reuse, second, no memory shifting.

I would also like to move the naming from owning/non-owning to 
variable/explicitParam and get rid of the MBool argument. That way, it's obvious
in which case to use which API. I add a variable, I add a variable, I don't have
to bother which ownership model this might have.

bindVariable(aName, aExprResult); and
/*
 * explicitly given parameters to a template are owned by the
 * parameter map.
 */
addExplicitParam(aName, aExprResult);

Same for names of the member vars.

This could be a all inline header file, I guess.
(no recursion, no multiple return points, that are the rules, AFAICT)
Attached patch new version (obsolete) — — Splinter Review 
This one fixes all Pikes comments except that I simply removed the support for
global parameters, once we know where/how the parametervalue will live we'll
deal with adding the code to handle it then. I also didn't split up
owning/nonowning into two functions. IMHO a function should be named after what
it does, not where it should be used. The clients of the function should know
what they want to do and call the appropriate functions to get it done.
Attachment #88190 - Attachment is obsolete: true
Comment on attachment 91098 [details] [diff] [review]
new version

VariableMap is still in transformiix.xml ;-)

<...>
>Index: source/xslt/ProcessorState.h
>===================================================================

>@@ -41,6 +41,8 @@
> #include "txIXPathContext.h"
> #include "txExpandedNameMap.h"
> #include "XSLTFunctions.h"
>+#include "txError.h"
>+#include "txVariableMap.h"

don't include txVariableMap here, but only in ProcessorState.cpp and
XSLTProcessor.cpp, instead
class txVariableMap;
here

<...>
>Index: source/xslt/XSLTProcessor.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xslt/XSLTProcessor.cpp,v
>retrieving revision 1.106
>diff -u -r1.106 XSLTProcessor.cpp
>--- source/xslt/XSLTProcessor.cpp	10 Jul 2002 05:04:20 -0000	1.106
>+++ source/xslt/XSLTProcessor.cpp	12 Jul 2002 14:07:59 -0000
>@@ -44,7 +44,7 @@
> #include "XSLTProcessor.h"
> #include "Names.h"
> #include "XMLParser.h"
>-#include "VariableBinding.h"
>+#include "txVariableMap.h"
> #include "XMLUtils.h"
> #include "XMLDOMUtils.h"
> #include "txNodeSorter.h"
>@@ -430,6 +430,8 @@
>                                     txListIterator* importFrame,
>                                     ProcessorState* aPs)
> {
>+    nsresult rv;
>+    <<<
whitespace, and yes, I like my rv = NS_OK. This one really, as it's used
in some ifs and not in others.

<...>
>@@ -523,16 +525,12 @@
>         }
>         // xsl:param
>         else if (localName == txXSLTAtoms::param) {
>-            String name;
>-            element->getAttr(txXSLTAtoms::name, kNameSpaceID_None,
>-                             name);
>-            if (name.isEmpty()) {
>-                String err("missing required name attribute for xsl:param");
>+            // Once we support stylesheet parameters we should process it here
// http://bugzilla.mozilla.org/show_bug.cgi?id=73492
// default is just a global variable

>+            rv = aPs->addGlobalVariable(element, currentFrame);
>+            if (NS_FAILED(rv)) {
>+                String err("unable to add global xsl:param");
>                 aPs->receiveError(err, NS_ERROR_FAILURE);
>-                break;

<...>

>+nsresult XSLTProcessor::processParameters(Element* aAction,
>+                                          Node* aContext,
>+                                          txVariableMap* aMap,
>+                                          ProcessorState* aPs)
> {
<...>
>+
>+            ExprResult* exprResult = processVariable(aContext, action, aPs);
>+

null check, I just had a 
if (exprResult) {
>+            rv = aMap->bindVariable(paramName, exprResult, MB_TRUE);
}
else {
    rv = NS_ERROR_FAILURE;
}
>+            if (NS_FAILED(rv)) {
>+                String err("Unable to bind parameter '");
>+                err.append(qName);
>+                err.append("'");
>+                aPs->receiveError(err, NS_ERROR_FAILURE);
>+                return rv;
<...>

>+/*
>+ * Processes the specified template using the given context,
>+ * ProcessorState, and parameters.
>+ * @param aTemplate the node in the xslt document that contains the
>+ *                  template
>+ * @param aContext  the current context node
>+ * @param aParams   map with parameters to the template
>+ * @param aPs       the current ProcessorState
>+ */
>+void XSLTProcessor::processTemplate(Node* aContext, Node* aTemplate,
>+                                    txVariableMap* aParams, ProcessorState* aPs)

order the comments like the arguments. Same in the header.


For inlining to work, this shouldn't have multiple return points, nor recursion

Please put the methods below the definition, as in
http://lxr.mozilla.org/seamonkey/source/extensions/transformiix/source/base/txM
ozillaString.h
for example.


>Index: source/xslt/txVariableMap.h
>===================================================================
>RCS file: source/xslt/txVariableMap.h
>diff -N source/xslt/txVariableMap.h
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ source/xslt/txVariableMap.h	12 Jul 2002 14:07:59 -0000
>@@ -0,0 +1,92 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is TransforMiiX XSLT processor.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Jonas Sicking.
>+ * Portions created by the Initial Developer are Copyright (C) 2002
>+ * Jonas Sicking. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Jonas Sicking <sicking@bigfoot.com>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#ifndef TRANSFRMX_VARIABLEMAP_H
>+#define TRANSFRMX_VARIABLEMAP_H
>+
>+#include "txError.h"
>+#include "XMLUtils.h"
>+#include "ExprResult.h"
>+#include "txExpandedNameMap.h"
>+
>+class txVariableMap {
>+public:
>+    txVariableMap(txVariableMap* aParentMap) : mParentMap(aParentMap),
>+                                               mOwnedVariables(MB_TRUE),
>+                                               mNonOwnedVariables(MB_FALSE)
>+    {
>+    }
>+    
>+    nsresult bindVariable(const txExpandedName& aName,
>+                          ExprResult* aValue, MBool aOwned)
>+    {
>+        if (getVariable(aName)) {
>+            return NS_ERROR_FAILURE;
>+        }
>+        if (aOwned) {
>+            return mOwnedVariables.add(aName, aValue);
>+        }
>+        return mNonOwnedVariables.add(aName, aValue);
>+    }
>+
>+    ExprResult* getVariable(const txExpandedName& aName)
>+    {
>+        ExprResult* var;
>+        if ((var = (ExprResult*)mOwnedVariables.get(aName))) {
>+            return var;
>+        }
>+        if ((var = (ExprResult*)mNonOwnedVariables.get(aName))) {
>+            return var;
>+        }
>+        if (mParentMap)
>+            return mParentMap->getVariable(aName);
>+        return 0;
>+    }
>+
>+private:
>+    // Parent map of variables
>+    txVariableMap* mParentMap;
>+
>+    // Map with owned variables
>+    txExpandedNameMap mOwnedVariables;
>+
>+    // Map with non-owned variables
>+    txExpandedNameMap mNonOwnedVariables;
>+};
>+

inline nsresult bindVariable(const txExpandedName& aName,
			     ExprResult* aValue, MBool aOwned)
{
    TxObject* var;
    var = mOwnedVariables.get(aName);
    if (!var) {
	var = mNonOwnedVariables.get(aName);
    }
    txVariableMap* parentMap = mParentMap;
    while (!var && parentMap)
	var = parentMap->mOwnedVariables.get(aName);
	if (!var) {
	    var = parentMap->mNonOwnedVariables.get(aName);
	}
    }
    nsresult rv = NS_ERROR_FAILURE;
    if (!var) {
	if (aOwned) {
	    rv = mOwnedVariables.add(aName, aValue);
	}
	else {
	    rv = mNonOwnedVariables.add(aName, aValue);
	}
    }
    return rv;
}

inline ExprResult* txVariableMap::getVariable(const txExpandedName& aName)
{
    ExprResult* var;
    var = (ExprResult*)mOwnedVariables.get(aName);
    if (!var) {
	var = (ExprResult*)mNonOwnedVariables.get(aName);
    }
    txVariableMap* parentMap = mParentMap;
    while (!var && parentMap)
	var = (ExprResult*)parentMap->mOwnedVariables.get(aName);
	if (!var) {
	    var = (ExprResult*)parentMap->mNonOwnedVariables.get(aName);
	}
    }
    return var;
}

>+#endif //TRANSFRMX_VARIABLEMAP_H

this way there is no recursion and no inlines calling inlines. It's just nicer
to compilers.

With this, r=axel@pike.org
Attachment #91098 - Flags: review+
Attached patch another (obsolete) — — Splinter Review 
fixed all comments.

I don't like the txVariableMap functions, the reason i put the implementation
in the .h file was not to get it inlined but rather to avoid the extra .cpp
which you didn't seem to like. Since the functions are so big inlineing them
will not buy us anything except a bigger binary. I thought that the entire idea
with using txExpandedNameMap was to get codereuse, so copying the getVariable
logic seems counter-producting to me. But i Just Don't Care Any More (tm), I
just want this patch checked in so i can get on with other things.
Attachment #90792 - Attachment is obsolete: true
Attachment #91098 - Attachment is obsolete: true
Keywords: review
Target Milestone: mozilla1.1beta → mozilla1.2alpha
Comment on attachment 91200 [details] [diff] [review]
another

> +        String err("missplaced xsl:param");

One 's' in "misplaced"

sr=bzbarsky
Attachment #91200 - Flags: superreview+
checked in!!!
Whiteboard: TX_BRIDGE_1_1_FIXED
Comment on attachment 91200 [details] [diff] [review]
another

checked in
Attachment #91200 - Attachment is obsolete: true
Keeping this open to fix the remaining issue of having ::processAction loop
through all siblings and create the variable-map only when needed
Priority: P2 → P5
Target Milestone: mozilla1.2alpha → Future
I'll do the rest of this as part of bug 185797
Depends on: xslt_compile
I think everybody should be happy with the variable-handling now. We only create
the txVariableMap when there is a variable/parameter, and we don't nest them anymore
Status: ASSIGNED → 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: