363242 - Make txExpandedNameMap typesafe

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

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

Use templates to make txExpandedNameMap typesafe. This way we can also remove the need for all things put in there to extend txObject.

Comment 1

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

Attachment: Patch to fix

This should do it. I also removed the mOwnsValues member and used separate classes for owning and non-owning maps instead. This also removed the need for the stored items to inherit txObject.

Additionally i changed a couple txLists into nsTArray

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 248053 [details] [diff] [review]
Patch to fix

Excellent, I'd just been looking at doing this :-). Next time, please compile before attaching a patch.

>Index: src/base/txExpandedNameMap.cpp
>===================================================================

>+class nsMapItemComparator

Maybe stick to the tx prefix?

>+nsresult txExpandedNameMap_base::addItem(const txExpandedName& aKey, void* aValue)

Rewrap.

>+    MapItem* item = mItems.AppendElement();
>+    NS_ENSURE_TRUE(item, NS_ERROR_OUT_OF_MEMORY);

I'd like to make you stick to existing style and make you not use NS_ENSURE_TRUE :-D.

>Index: src/base/txExpandedNameMap.h
>===================================================================

>+    nsresult setItem(const txExpandedName& aKey, void* aValue, void** aOldValue);

Rewrap.

>+        iterator_base(txExpandedNameMap_base& aMap)
>+            : mMap(aMap),
>+              mCurrentPos(-1)

Please cast to PRUint32 to silence compiler warning or use PR_UINT32_MAX.

>+class txExpandedNameMap : public txExpandedNameMap_base

>+    void clear()
>+    {
>+        clearItems()

Missing semi-colon.

>Index: src/xslt/txKeyFunctionCall.cpp
>===================================================================

>@@ -308,46 +308,34 @@ txKeyHash::init()
>     
>     return NS_OK;
> }
> 
> /**
>  * Class holding all <xsl:key>s of a particular expanded name in the
>  * stylesheet.
>  */

Nit: you can remove that comment too IMHO.

>-txXSLKey::~txXSLKey()

>Index: src/xslt/txStylesheet.cpp
>===================================================================

>@@ -414,27 +406,27 @@ txStylesheet::addTemplate(txTemplateItem

>+        nsAutoPtr<nsTArray<MatchableTemplate> > newList(

Nit: either nsAutoPtr< nsTArray<MatchableTemplate> > or nsAutoPtr<nsTArray<MatchableTemplate>>

>@@ -446,38 +438,36 @@ txStylesheet::addTemplate(txTemplateItem

>+        for (i = 0; i < len; ++i) {
>+            MatchableTemplate& mt = (*templates)[i];
>+            if (priority > mt.mPriority) {
>+                nt = templates->InsertElementAt(i);
>+                NS_ENSURE_TRUE(nt, NS_ERROR_OUT_OF_MEMORY);

>+        if (!nt) {
>+            nt = templates->AppendElement();
>+            NS_ENSURE_TRUE(nt, NS_ERROR_OUT_OF_MEMORY);
>         }

Or you could break out of the loop if |priority > mt.mPriority| and always InsertElementAt(i).

>Index: src/xslt/txStylesheet.h
>===================================================================

>     class ImportFrame {
>     public:
>         ImportFrame();

Need to remove the semi-colon.

>+            : mFirstNotImported(nsnull)
>+        {
>+        }

>+        txOwningExpandedNameMap<nsTArray<MatchableTemplate> > mMatchableTemplates;


Nit: either txOwningExpandedNameMap< nsTArray<MatchableTemplate> > or txOwningExpandedNameMap<nsTArray<MatchableTemplate>>

>@@ -234,16 +228,16 @@ public:

>-class txIGlobalParameter : public TxObject
>+class txIGlobalParameter

Why do we need this, can't we use txVariable everywhere?

>Index: src/xslt/txStylesheetCompileHandlers.cpp
>===================================================================

> txHandlerTable::init(const txElementHandler* aHandlers, PRUint32 aCount)

>+        rv = mHandlers.add(name, NS_CONST_CAST(txElementHandler*, aHandlers));

You could do away with this cast by adding an old-style cast to void* for aValue in txExpandedNameMap::add. Then mHandlers can be a |txExpandedNameMap<const txElementHandler>|

Comment 3

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

(In reply to comment #2)
> (From update of attachment 248053 [details] [diff] [review] [edit])
> Excellent, I'd just been looking at doing this :-). Next time, please compile
> before attaching a patch.

Ugh, must have forgot to do a final diff before attaching. However MSVC compiles fine without the missing ';' in clear(), probably because the function is never instantiated.

> >+    MapItem* item = mItems.AppendElement();
> >+    NS_ENSURE_TRUE(item, NS_ERROR_OUT_OF_MEMORY);
> 
> I'd like to make you stick to existing style and make you not use
> NS_ENSURE_TRUE :-D.

The style of this file is to use NS_ENSURE :P
(also, not using NS_ENSURE is stupid. IMHO of course ;) )

> >Index: src/xslt/txStylesheet.cpp
> >===================================================================
> 
> >@@ -414,27 +406,27 @@ txStylesheet::addTemplate(txTemplateItem
> 
> >+        nsAutoPtr<nsTArray<MatchableTemplate> > newList(
> 
> Nit: either nsAutoPtr< nsTArray<MatchableTemplate> > or
> nsAutoPtr<nsTArray<MatchableTemplate>>

nsAutoPtr<nsTArray<MatchableTemplate>> won't compile since '>>' will be lexed as a shift operator, not as two '>'.

> >@@ -234,16 +228,16 @@ public:
> 
> >-class txIGlobalParameter : public TxObject
> >+class txIGlobalParameter
> 
> Why do we need this, can't we use txVariable everywhere?

txVariable is module-only since it contains nsIVariable stuff. However we could probably make txVariable be the main class and use #ifdefs inside it. That's a different bug though.

Comment 4

[Peter Van der Beken [:peterv]]

Should we un-inline txOwningExpandedNameMap::clear?

 	       +424 (+4692/-4268)	text (DATA)
  		       +424 (+4692/-4268)	UNDEF:firefox-bin:text
  			      +1148	txStylesheet::~txStylesheet()
 

Comment 5

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

We can't since it's templated. Well.. we could if we required all stored objects to be TxObjects again.

Comment 6

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

This was checked in. Thanks for the quick review