Closed Bug 256744 Opened 20 years ago Closed 20 years ago

Integrate Novell master dependency graph code

Categories

(Core Graveyard :: XForms, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bryner, Assigned: allan)

Details

Attachments

(1 file, 4 obsolete files)

Tracking bug for integrating Novell's master dependency graph code for XForms.
Assignee: aaronr → allan
Status: NEW → ASSIGNED
Attached patch MDG Patch 1 (obsolete) — Splinter Review
Ok, here's a "first edition". It lacks a couple of XPath functionalities, for
which the location is a bit undecided for now. It also needs to be tied
properly together with the new design of <model> and <bind>, but for the
current code it is integrated as much as possible.

nsXFormsMDGEngine.cpp is also filled with a lot of |#ifdef
DEBUG_beaufour|-sections... these will go away later in the process :)
Could you use 2-space indention?
Ah, sorry. Didn't check the --expand-tabs output from diff. I'll fix it, please
take on your "ignore-indent-error" glasses on when reading :)
And for debugging no need for DumpDocument(). 
Just use nsIDOMSerializer or nsIDOMLSSerializer.
Attached patch MDG Patch 2 (obsolete) — Splinter Review
New patch for y'all :) 2-space indent, and goodbye to DumpDocument (and thus
nsXFormsUtils.h and .cpp)
Attachment #159595 - Attachment is obsolete: true
Comment on attachment 159704 [details] [diff] [review]
MDG Patch 2

>--- ../xforms.orig/nsXFormsMDG.cpp  1970-01-01 01:00:00.000000000 +0100
>+++ ./nsXFormsMDG.cpp 2004-09-21 19:41:53.000000000 +0200
>+nsresult
>+nsXFormsMDG::CreateNewChild(nsCOMPtr<nsIDOMNode> aContextNode, const nsAString& aNodeValue,
>+                            nsCOMPtr<nsIDOMNode> aBeforeNode)

nsCOMPtr parameters should generally be avoided... especially the way it's
written here, it will copy the nsCOMPtr which will result in an extra
addref/release.  Just use the raw pointer type.  This applies for several of
the methods.

>+{
>+ nsresult rv;

please two-space indent, not one.

>+
>+ nsCOMPtr<nsIDOMDocument> document;
>+ rv = aContextNode->GetOwnerDocument(getter_AddRefs(document));
>+ NS_ENSURE_SUCCESS(rv, rv);
>+
>+ nsCOMPtr<nsIDOMText> textNode;
>+ rv = document->CreateTextNode(aNodeValue, getter_AddRefs(textNode));
>+ NS_ENSURE_SUCCESS(rv, rv);
>+ 
>+ nsCOMPtr<nsIDOMNode> newNode;
>+ if (aBeforeNode) {
>+   rv = aContextNode->InsertBefore(textNode, aBeforeNode, getter_AddRefs(newNode));
>+ } else {
>+   rv = aContextNode->AppendChild(textNode, getter_AddRefs(newNode));
>+ }
>+ NS_ENSURE_SUCCESS(rv, rv);
>+
>+ return NS_OK;

I think you can just return rv here.

>+nsresult
>+nsXFormsMDG::AddMIP(ModelItemPropName aType, nsCOMPtr<nsIDOMXPathExpression> aExpression,
>+                    nsCOMPtr<nsIDOMNode> aContextNode, PRInt32 aContextPos, PRInt32 aContextSize)
>+{
>+ NS_ENSURE_ARG(aExpression);
>+ NS_ENSURE_ARG(aContextNode);
>+ 
>+ nsVoidArray deps;
>+ printf("\tTODO: Get dependencies!\n");

This should be commented out before checkin.

>+nsresult
>+nsXFormsMDG::GetNodeValue(nsCOMPtr<nsIDOMNode> aContextNode, nsAString& aNodeValue)
>+{
...

>+ case nsIDOMNode::ELEMENT_NODE:
>+   rv = aContextNode->GetFirstChild(getter_AddRefs(childNode));

How is this method going to be used? I couldn't find any callers.  Depending on
how it will be used, you may want to consider nsIDOM3Node::GetTextContent here,
which will handle cases like:

<blah>some text<!-- comment -->more text</blah>

>+   if (NS_FAILED(rv) || !childNode) {
>+     // No child
>+     aNodeValue.Assign(NS_LITERAL_STRING(""));;

aNodeValue.Truncate(0);

>+   } else {
>+     PRUint16 childType;
>+     rv = childNode->GetNodeType(&childType);
>+     NS_ENSURE_SUCCESS(rv, rv);
>+ 
>+     if (   childType == nsIDOMNode::TEXT_NODE
>+         || childType == nsIDOMNode::CDATA_SECTION_NODE) {
>+       rv = childNode->GetNodeValue(aNodeValue);
>+       NS_ENSURE_SUCCESS(rv, rv);
>+     } else {
>+       // Not a text child
>+       aNodeValue.Assign(NS_LITERAL_STRING(""));

same.

>+nsresult
>+nsXFormsMDG::SetNodeValue(nsCOMPtr<nsIDOMNode> aContextNode, nsAString& aNodeValue, PRBool aMarkNode)
>+{
...
>+ case nsIDOMNode::ELEMENT_NODE:
>+   rv = aContextNode->GetFirstChild(getter_AddRefs(childNode));
>+   NS_ENSURE_SUCCESS(rv, rv);
>+
>+   if (!childNode) {
>+     rv = CreateNewChild(aContextNode, aNodeValue);
>+     NS_ENSURE_SUCCESS(rv, rv);

You can also use nsIDOM3Node::SetTextContent here, if that would be helpful.

>+PRBool
>+nsXFormsMDG::IsValid(nsCOMPtr<nsIDOMNode> aContextNode)
>+{
>+ 
>+ PRBool valid = mEngine.Test(aContextNode, MDG_FLAG_CONSTRAINT);
>+ if (valid == PR_TRUE) {
>+   printf("TODO: nsXFormsMDG::isValid() needs Schema support\n");

remove this before checking in

>+   // valid = mSchemaHandler->ValidateNode(aContextNode);
>+ }
>+ return valid;
>+}
>+
>+PRBool
>+nsXFormsMDG::IsRelevant(nsCOMPtr<nsIDOMNode> aContextNode)
>+{
>+ PRBool valid;
>+ if (   mEngine.Test(aContextNode, MDG_FLAG_RELEVANT) == PR_TRUE
>+     && mEngine.Test(aContextNode, MDG_FLAG_INHERITED_RELEVANT) == PR_TRUE) {

It might be good to make the values of the MDG_FLAG_* constants be the actual
bits, then you could do something like:

mEngine.Test(aContextNode, MDG_FLAG_RELEVANT | MDG_FLAG_INHERITED_RELEVANT)

if you change nsXFormsMDGEngine::Test to do this:

 return (GetFlag(aDomNode) & aFlagId == aFlagId)

You also avoid doing all the bit-shifting at run-time.

I'm up to nsXFormsMDGEngine.cpp, will add more comments later.
Attached patch MDG Patch 3 (obsolete) — Splinter Review
Voila, latest patch.
* Ported our XPath analyzer to get the dependencies for the XPath expressions.
It should only be seen as a "showcase". It really needs to be implemented in
the Transformiix code.
* Implemented the review comments from Brian and Aaron.
Attachment #159704 - Attachment is obsolete: true
Comment on attachment 161009 [details] [diff] [review]
MDG Patch 3

>--- ../xforms.orig/nsXFormsMDGEngine.cpp	1970-01-01 01:00:00.000000000 +0100
>+++ ./nsXFormsMDGEngine.cpp	2004-10-04 15:15:30.961126336 +0200
>+PLDHashOperator
>+nsXFormsMDGEngine::DeleteLinkedNodes(const nsISupportsHashKey::KeyType aKey, nsAutoPtr<nsXFormsMDGNode>& aNode, void* aArg)
>+{

How about:

(const nsISupports *aKey, nsXFormsMDGNode *aNode, void *aArg)

The first argument you have there is technically equivalent, but the intent was
that you write the raw pointer form as it's a lot easier.  Not sure why the
second argument actually compiles as an nsAutoPtr.

>+nsresult
>+nsXFormsMDGEngine::Finalize()
>+{
>+  nsresult rv = NS_ERROR_FAILURE;
>+  if (mNodeToFlag.Init() && mNodeToMDG.Init()) {
>+    rv = NS_OK;

No point in setting rv here (or having it at all) if you're not going to do
anything with it.

>+  }
>+  
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsXFormsMDGEngine::Insert(nsIDOMNode* aContextNode, nsIDOMXPathExpression* aExpression,
>+                          const nsXFormsMDGSet* aDependencies, PRBool aDynFunc,
>+                          PRInt32 aContextPos, PRInt32 aContextSize,
>+                          ModelItemPropName aType, ModelItemPropName aDepType)
>+{
...
>+  if (newnode->HasExpr()) {
>+    // MultipleMIPs
>+    return NS_ERROR_ABORT;

What does this mean, exactly? Multiple MIPs are legal... what case is this
handling?

>+PLDHashOperator 
>+nsXFormsMDGEngine::AddStartNodes(const nsISupportsHashKey::KeyType aKey, nsXFormsMDGNode* aNode, void* aDeque)

Same comment as above regarding the first parameter type.

>+  // Initial scan for nsXFormsMDGNodes with no dependencies (count == 0)
>+  PRUint32 entries = mNodeToMDG.EnumerateRead(AddStartNodes, &sortedNodes);
>+  if (entries != mNodeToMDG.Count()) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
>+
>+#ifdef DEBUG_XF_MDG
>+  printf("\tStartNodes:    %d\n", sortedNodes.GetSize());
>+#endif
>+  
>+  nsXFormsMDGNode* g;
>+  nsXFormsMDGNode* n;
>+  void* nextNode = sortedNodes.Pop();

How about:

nsXFormsMDGNode *nextNode = NS_STATIC_CAST(nsXFormsMDGNode*,
sortedNodes.Pop());

and get rid of 'g'

>+  while (nextNode != nsnull) {
>+    g = NS_STATIC_CAST(nsXFormsMDGNode*, nextNode);
>+    if (!g) {
>+      return NS_ERROR_OUT_OF_MEMORY;

static_cast always succeeds, no need for this check.

>+    }
>+    for (PRInt32 i = 0; i < g->mSuc.Count(); ++i) {
>+      n = NS_STATIC_CAST(nsXFormsMDGNode*, g->mSuc[i]);

Scope 'n' inside this for block, and maybe rename it to 'node'.

>+      if (!n) {
>+        return NS_ERROR_OUT_OF_MEMORY;

This isn't an OOM condition or likely to ever happen (you don't allow appending
a null item to this array). I would just have an NS_ASSERTION.

>+      } else { // The nodes are becomming relevant

'becoming'

>+nsresult
>+nsXFormsMDGEngine::Calculate(nsXFormsMDGSet& aValueChanged)
>+{
>+#ifdef DEBUG_XF_MDG
>+  printf("nsXFormsMDGEngine::Calculcate(aValueChanged=|%d|)\n", aValueChanged.Count());
>+#endif
>+
>+  NS_ENSURE_TRUE(aValueChanged.AddSet(mMarkedNodes), NS_ERROR_OUT_OF_MEMORY);
>+
>+  mMarkedNodes.Clear();
>+  
>+  PRBool res = PR_TRUE;
>+  
>+  if (mJustRebuilded) {
>+    mFirstCalculate = PR_TRUE;
>+  } else {
>+    mFirstCalculate = PR_FALSE;
>+  }

mFirstCalculate = mJustRebuilded;

>+// TODO: This function needs to be called 
>+nsresult
>+nsXFormsMDGEngine::MarkNode(nsIDOMNode* aDomNode)
>+{
>+  SetFlagBits(aDomNode, MDG_FLAG_ALL_DISPATCH, PR_TRUE);
>+
>+  nsXFormsMDGNode* n = GetNode(aDomNode, eModel_calculate);
>+  if (n != nsnull) {
>+    n->MarkDirty();
>+  }
>+
>+  // Add constraint to trigger validation of node 
>+  n = GetNode(aDomNode, eModel_constraint, PR_FALSE);
>+  if (n == nsnull) {
>+    n = GetNode(aDomNode, eModel_constraint, PR_TRUE);
>+    if (!n) {

Try to be consistent with (!n) for null checks, vs (n == nsnull)

>--- ../xforms.orig/nsXFormsMDGEngine.h	1970-01-01 01:00:00.000000000 +0100
>+++ ./nsXFormsMDGEngine.h	2004-10-04 15:11:22.985824312 +0200
>+  /**
>+   * True when Rebuild() has been run, but not ClearDispatchFlags()
>+   */
>+  PRBool mJustRebuilded;

Can we make this mJustRebuilt, so it doesn't distract me when I'm reading this
code? :)

>--- ../xforms.orig/nsXFormsModelElement.cpp	2004-10-02 00:06:50.000000000 +0200
>+++ ./nsXFormsModelElement.cpp	2004-10-04 15:23:36.526309208 +0200
>@@ -501,6 +497,9 @@
> 
>   NS_ASSERTION(mContent, "Wrapper is not an nsIContent, we'll crash soon");
> 
>+  nsresult rv = mMDG.Finalize();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

Finalize seems like kind of an odd name for this, since it sounds like we're
finished with something.... Init() would be more readable for me.

I'm going to skip review on the XPath stuff since it sounds like it will be
rewritten.

I'm happy with this patch other than the above remarks... fix those and I'll r+
and check it in.
Attachment #161009 - Flags: review-
(In reply to comment #8)
> (From update of attachment 161009 [details] [diff] [review])
> >--- ../xforms.orig/nsXFormsMDGEngine.cpp	1970-01-01 01:00:00.000000000 +0100
> >+++ ./nsXFormsMDGEngine.cpp	2004-10-04 15:15:30.961126336 +0200
> >+PLDHashOperator
> >+nsXFormsMDGEngine::DeleteLinkedNodes(const nsISupportsHashKey::KeyType aKey,
nsAutoPtr<nsXFormsMDGNode>& aNode, void* aArg)
> >+{
> 
> How about:
> 
> (const nsISupports *aKey, nsXFormsMDGNode *aNode, void *aArg)
> 
> The first argument you have there is technically equivalent, but the intent was
> that you write the raw pointer form as it's a lot easier.  Not sure why the
> second argument actually compiles as an nsAutoPtr.

Hmm, the datatype needed for the Enumerate function is:
PRUint32 
   nsBaseHashtable<KeyClass, DataType, UserDataType>::Enumerate(PLDHashOperator 
   (*)(typename KeyClass::KeyType, DataType&, void*), void*) [with KeyClass = 
   nsISupportsHashKey, DataType = nsAutoPtr<nsXFormsMDGNode>, UserDataType = 
   nsXFormsMDGNode*]

Which is exactly what I have. Am I missing something here?

> >+nsresult
> >+nsXFormsMDGEngine::Finalize()
> >+{
> >+  nsresult rv = NS_ERROR_FAILURE;
> >+  if (mNodeToFlag.Init() && mNodeToMDG.Init()) {
> >+    rv = NS_OK;
> 
> No point in setting rv here (or having it at all) if you're not going to do
> anything with it.
> >+  }
> >+  
> >+  return NS_OK;
> >+}

:) Correct, "return rv;" of course.

> >+nsresult
> >+nsXFormsMDGEngine::Insert(nsIDOMNode* aContextNode, nsIDOMXPathExpression*
aExpression,
> >+                          const nsXFormsMDGSet* aDependencies, PRBool aDynFunc,
> >+                          PRInt32 aContextPos, PRInt32 aContextSize,
> >+                          ModelItemPropName aType, ModelItemPropName aDepType)
> >+{
> ...
> >+  if (newnode->HasExpr()) {
> >+    // MultipleMIPs
> >+    return NS_ERROR_ABORT;
> 
> What does this mean, exactly? Multiple MIPs are legal... what case is this
> handling?

Multiple MIP of the same type is not allowed, as in "<bind nodeset="x"
required="true()" required="false()"/>" Added a better comment.

> >+PLDHashOperator 
> >+nsXFormsMDGEngine::AddStartNodes(const nsISupportsHashKey::KeyType aKey,
nsXFormsMDGNode* aNode, void* aDeque)
> 
> Same comment as above regarding the first parameter type.

Same answer :)

> >+  // Initial scan for nsXFormsMDGNodes with no dependencies (count == 0)
> >+  PRUint32 entries = mNodeToMDG.EnumerateRead(AddStartNodes, &sortedNodes);
> >+  if (entries != mNodeToMDG.Count()) {
> >+    return NS_ERROR_OUT_OF_MEMORY;
> >+  }
> >+
> >+#ifdef DEBUG_XF_MDG
> >+  printf("\tStartNodes:    %d\n", sortedNodes.GetSize());
> >+#endif
> >+  
> >+  nsXFormsMDGNode* g;
> >+  nsXFormsMDGNode* n;
> >+  void* nextNode = sortedNodes.Pop();
> 
> How about:
> 
> nsXFormsMDGNode *nextNode = NS_STATIC_CAST(nsXFormsMDGNode*,
> sortedNodes.Pop());
> 
> and get rid of 'g'

Check.

> >+  while (nextNode != nsnull) {
> >+    g = NS_STATIC_CAST(nsXFormsMDGNode*, nextNode);
> >+    if (!g) {
> >+      return NS_ERROR_OUT_OF_MEMORY;
> 
> static_cast always succeeds, no need for this check.

I dunno why I had that check... it's gone.

> >+    }
> >+    for (PRInt32 i = 0; i < g->mSuc.Count(); ++i) {
> >+      n = NS_STATIC_CAST(nsXFormsMDGNode*, g->mSuc[i]);
> 
> Scope 'n' inside this for block, and maybe rename it to 'node'.

Hmm, I'm not to fond of having declarations inside loops. Maybe I'm too
oldfashioned? I renamed the variables.

> >+      if (!n) {
> >+        return NS_ERROR_OUT_OF_MEMORY;
> 
> This isn't an OOM condition or likely to ever happen (you don't allow appending
> a null item to this array). I would just have an NS_ASSERTION.

Check.

> >+      } else { // The nodes are becomming relevant
> 
> 'becoming'

Check.

> >+nsresult
> >+nsXFormsMDGEngine::Calculate(nsXFormsMDGSet& aValueChanged)
> >+{
> >+#ifdef DEBUG_XF_MDG
> >+  printf("nsXFormsMDGEngine::Calculcate(aValueChanged=|%d|)\n",
aValueChanged.Count());
> >+#endif
> >+
> >+  NS_ENSURE_TRUE(aValueChanged.AddSet(mMarkedNodes), NS_ERROR_OUT_OF_MEMORY);
> >+
> >+  mMarkedNodes.Clear();
> >+  
> >+  PRBool res = PR_TRUE;
> >+  
> >+  if (mJustRebuilded) {
> >+    mFirstCalculate = PR_TRUE;
> >+  } else {
> >+    mFirstCalculate = PR_FALSE;
> >+  }
> 
> mFirstCalculate = mJustRebuilded;

Wow, I'm blind :)

> >+// TODO: This function needs to be called 
> >+nsresult
> >+nsXFormsMDGEngine::MarkNode(nsIDOMNode* aDomNode)
> >+{
> >+  SetFlagBits(aDomNode, MDG_FLAG_ALL_DISPATCH, PR_TRUE);
> >+
> >+  nsXFormsMDGNode* n = GetNode(aDomNode, eModel_calculate);
> >+  if (n != nsnull) {
> >+    n->MarkDirty();
> >+  }
> >+
> >+  // Add constraint to trigger validation of node 
> >+  n = GetNode(aDomNode, eModel_constraint, PR_FALSE);
> >+  if (n == nsnull) {
> >+    n = GetNode(aDomNode, eModel_constraint, PR_TRUE);
> >+    if (!n) {
> 
> Try to be consistent with (!n) for null checks, vs (n == nsnull)

Check. Should be fixed now.

> >--- ../xforms.orig/nsXFormsMDGEngine.h	1970-01-01 01:00:00.000000000 +0100
> >+++ ./nsXFormsMDGEngine.h	2004-10-04 15:11:22.985824312 +0200
> >+  /**
> >+   * True when Rebuild() has been run, but not ClearDispatchFlags()
> >+   */
> >+  PRBool mJustRebuilded;
> 
> Can we make this mJustRebuilt, so it doesn't distract me when I'm reading this
> code? :)

:) Yes.

> >--- ../xforms.orig/nsXFormsModelElement.cpp	2004-10-02 00:06:50.000000000 +0200
> >+++ ./nsXFormsModelElement.cpp	2004-10-04 15:23:36.526309208 +0200
> >@@ -501,6 +497,9 @@
> > 
> >   NS_ASSERTION(mContent, "Wrapper is not an nsIContent, we'll crash soon");
> > 
> >+  nsresult rv = mMDG.Finalize();
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> 
> Finalize seems like kind of an odd name for this, since it sounds like we're
> finished with something.... Init() would be more readable for me.

Well, a question of terminology, but Init() seems like the Mozilla-way, I'll
change it.

> I'm going to skip review on the XPath stuff since it sounds like it will be
> rewritten.
> 
> I'm happy with this patch other than the above remarks... fix those and I'll r+
> and check it in.

Sounds good!

I'll fix the above, a mem leak, and then upload a new patch.
Attached patch MDG Patch 4 (obsolete) — Splinter Review
Review comments integrated and mem leak fixed.
Attachment #161009 - Attachment is obsolete: true
(In reply to comment #9)
> Hmm, the datatype needed for the Enumerate function is:
> PRUint32 
>    nsBaseHashtable<KeyClass, DataType, UserDataType>::Enumerate(PLDHashOperator 
>    (*)(typename KeyClass::KeyType, DataType&, void*), void*) [with KeyClass = 
>    nsISupportsHashKey, DataType = nsAutoPtr<nsXFormsMDGNode>, UserDataType = 
>    nsXFormsMDGNode*]
> 
> Which is exactly what I have. Am I missing something here?

Well, in nsHashKeys.h, it has:

class NS_COM nsISupportsHashKey : public PLDHashEntryHdr
{
public:
  typedef nsISupports* KeyType;

So, the intent is that you can use a "natural" parameter type for the key.  For
nsISupportsHashKey, nsISupports*; for nsStringHashKey, const nsAString&.  It's a
lot more readable, and also lets you make sure the parameter type is what you
think it is.  (especially when const / references are involved)

> > >+    }
> > >+    for (PRInt32 i = 0; i < g->mSuc.Count(); ++i) {
> > >+      n = NS_STATIC_CAST(nsXFormsMDGNode*, g->mSuc[i]);
> > 
> > Scope 'n' inside this for block, and maybe rename it to 'node'.
> 
> Hmm, I'm not to fond of having declarations inside loops. Maybe I'm too
> oldfashioned? I renamed the variables.
> 

It's nice to scope variables at the smallest scope where they're used so that
you don't make a mistake using the variable later in the method.  This is the
normal coding style for mozilla.
(In reply to comment #11)
> (In reply to comment #9)
> > Hmm, the datatype needed for the Enumerate function is:
> > PRUint32 
> >    nsBaseHashtable<KeyClass, DataType, UserDataType>::Enumerate(PLDHashOperator 
> >    (*)(typename KeyClass::KeyType, DataType&, void*), void*) [with KeyClass = 
> >    nsISupportsHashKey, DataType = nsAutoPtr<nsXFormsMDGNode>, UserDataType = 
> >    nsXFormsMDGNode*]
> > 
> > Which is exactly what I have. Am I missing something here?
> 
> Well, in nsHashKeys.h, it has:
> 
> class NS_COM nsISupportsHashKey : public PLDHashEntryHdr
> {
> public:
>   typedef nsISupports* KeyType;
> 
> So, the intent is that you can use a "natural" parameter type for the key.

Why have a typedef, and furthermore use it in function signatures, etc. when you
are not supposed to use it? I strongly disagree on that one. I think using the
typedef is the most elegant and clean way to use it.

> For
> nsISupportsHashKey, nsISupports*; for nsStringHashKey, const nsAString&.  It's a
> lot more readable, and also lets you make sure the parameter type is what you
> think it is.  (especially when const / references are involved)

Yes, and in nsBaseHashtable.h the EnumerateRead function signature is:
  typedef PLDHashOperator
    (*PR_CALLBACK EnumReadFunction)(KeyType      aKey,
                                    UserDataType aData,
                                    void*        userArg);

And this is what my compiler expects, if you can convince it otherwise go ahead.  

> > > >+    }
> > > >+    for (PRInt32 i = 0; i < g->mSuc.Count(); ++i) {
> > > >+      n = NS_STATIC_CAST(nsXFormsMDGNode*, g->mSuc[i]);
> > > 
> > > Scope 'n' inside this for block, and maybe rename it to 'node'.
> > 
> > Hmm, I'm not to fond of having declarations inside loops. Maybe I'm too
> > oldfashioned? I renamed the variables.
> > 
> 
> It's nice to scope variables at the smallest scope where they're used so that
> you don't make a mistake using the variable later in the method.  This is the
> normal coding style for mozilla.

Depending on the variable type it could also be re-initialized in each
iteration, which could be quite unnecessary and expensive. But in the case of a
pointer, no harm done there :) It is changed now. But in general I am not to
fond of it, but if it is "the Mozilla way" ... once again I'll change the old
habits. I'll change the rest of the code, but after check in, ok?
Attached patch MDG Patch 5Splinter Review
* Fixed the declaration and names in nsXFormsMDGEngine::Rebuild()
* Brought up to date with current CVS
* Updated nsXFormsXPathXMLUtil to latest internal version
Attachment #161132 - Attachment is obsolete: true
Checked in, with the change of |const nsISupportsHashKey::KeyType| to
|nsISupports*| that we discussed.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: