Closed Bug 481881 Opened 11 years ago Closed 11 years ago

use better template arguments for nsTArray<T> after bug 474369

Categories

(Core :: General, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: Swatinem, Assigned: Swatinem)

References

Details

Attachments

(4 files, 9 obsolete files)

22.72 KB, patch
roc
: review+
Details | Diff | Splinter Review
23.66 KB, patch
roc
: review+
Details | Diff | Splinter Review
23.06 KB, patch
Swatinem
: review+
Swatinem
: superreview+
Details | Diff | Splinter Review
782 bytes, patch
roc
: review+
Details | Diff | Splinter Review
Attached patch widget part (obsolete) — Splinter Review
Per bug 474369 comment 33:

> +    nsTPtrArray<nsCString> mDataFlavors;
> 
> Mmm, that looks like a good candidate for conversion to nsTArray<nsCString>.
> Later.
> 
> +    nsTArray<nsString*> mDataFlavors;
> 
> That too!
> 
> +    nsTPtrArray<IDataObject> mDataObjects;

I changed some other cases to nsTArray<T> directly, where the elements are owned by the nsTArray.

Patch runs reftest fine on Linux, but is untested on windows however.
Attachment #365909 - Flags: superreview?(roc)
Attachment #365909 - Flags: review?(roc)
Attached patch layout part (obsolete) — Splinter Review
I found some more cases in layout.
Attachment #365952 - Flags: superreview?(roc)
Attachment #365952 - Flags: review?(roc)
Great stuff!

+  nsCOMPtr<nsISupports> mData;   // OWNER - some varient of primitive wrapper
+  PRUint32 mDataLen;
+  const nsCAutoString mFlavor;
+  char *   mCacheFileName;

There's a subtle bug here. You can't put objects containing an nsCAutoString directly into an nsTArray. The array can move its contents around in memory, and ns(C)AutoString objects (and nsAutoTArray objects) can contain an internal pointer to themselves, which is not updated when the object moves. Here I think you should just use an nsCString.

-  nsTArray<DataStruct*> * mDataArray;
+  nsTArray<DataStruct> * mDataArray;

Here, there is no benefit to heap-allocating the nsTArray. It should just be a direct member of the class.
Attached patch widget part, v2 (obsolete) — Splinter Review
Addressed the comments.
Attachment #365909 - Attachment is obsolete: true
Attachment #365968 - Flags: superreview?(roc)
Attachment #365968 - Flags: review?(roc)
Attachment #365909 - Flags: superreview?(roc)
Attachment #365909 - Flags: review?(roc)
+    DR_FrameTypeInfo info = mFrameTypeTable.ElementAt(i);

Use "DR_FrameTypeInfo& info" to avoid making a copy (2 occurrences)

+    DeepTreeStackItem top = mStack.ElementAt(mStack.Length()-1);

Similar here

+  if(!mBCInfo->mRightBorders.SetLength(aRowIndex+1))

Missing space after 'if'

You'd better document that nsTableCellMap::GetBCData returns a pointer into the array and that pointer should not be used after anything that could change the array (since that could move the data in memory). Actually, it would be better to remove GetBCData and replace it with a method that just sets SetTopStart(PR_FALSE) directly on the data, so we don't have to worry about what happens after we return the pointer.

+  if (!mCols.AppendElements(aNumCols))
+    NS_WARNING("Could not AppendElement");
+  if (mBCInfo) {
+    if (!mBCInfo->mBottomBorders.AppendElements(aNumCols))
+      NS_WARNING("Could not AppendElement");

The NS_WARNINGs should be in {} since they're not control transfers.

+            if(!mBCInfo->mRightBorders.InsertElementAt(rowX))

Need space after 'if' (2 occurrences)

+    nsColInfo colInfo = mCols.ElementAt(colX);

Use "nsColInfo& colInfo" to avoid a copy

+        BCData cd = mBCInfo->mBottomBorders.ElementAt(colIndex);

"BCData& cd" (2 occurrences)
-    nsCString * df = mDataFlavors.SafeElementAt(dfInx);
-    if ( df ) {

I don't think we should get rid of this if. Just check dfInx >= 0 && dfInx < mDataFlavours.Length()

+    nsCString df = mDataFlavors.SafeElementAt(dfInx, nsCString());

You want
  nsCString& df = mDataFlavors[dfInx];
now that we've checked the index is in bounds. Avoid copies!

+    nsCString df = mDataFlavors.SafeElementAt(dfInx, nsCString());

Here you can use nsCString& df = mDataFlavours.SafeElementAt(dfInx, EmptyCString()). Can't use nsCString() as the default value if we're going to hold a reference to it, since it will go away after this statement.

+#include "nsAutoPtr.h"

nsRefPtr.h?

+    ScreenListItem curr = mScreenList[i];

ScreenListItem& curr

+    DataStruct data = pArray->ElementAt (i);

DataStruct& data

+      return &pArray->ElementAt (i);

return &data. Although actually, I think GetDataForFlavor would work better if it just returned the index in the array or NoIndex if not found. Then RemoveDataFlavor can keep calling GetDataForFlavor.

+    DataStruct data = mDataArray.ElementAt(i);

DataStruct& data (many occurrences!)
Attached patch layout part, v2 (obsolete) — Splinter Review
Attachment #365952 - Attachment is obsolete: true
Attachment #366082 - Flags: superreview?(roc)
Attachment #366082 - Flags: review?(roc)
Attachment #365952 - Flags: superreview?(roc)
Attachment #365952 - Flags: review?(roc)
> +#include "nsAutoPtr.h"
> 
> nsRefPtr.h?

It actually is in nsAutoPtr.h :-P
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsAutoPtr.h#918
Attachment #365968 - Attachment is obsolete: true
Attachment #366083 - Flags: superreview?(roc)
Attachment #366083 - Flags: review?(roc)
Attachment #365968 - Flags: superreview?(roc)
Attachment #365968 - Flags: review?(roc)
Comment on attachment 366082 [details] [diff] [review]
layout part, v2

+  if (bcData)
+    bcData->SetTopStart(PR_FALSE);

Need {} around this statement since it's not a control transfer

There's a few "if(" you've added here too.
Attachment #366083 - Flags: superreview?(roc)
Attachment #366083 - Flags: superreview+
Attachment #366083 - Flags: review?(roc)
Attachment #366083 - Flags: review+
Attached patch layout part, v3 (obsolete) — Splinter Review
Attachment #366082 - Attachment is obsolete: true
Attachment #366215 - Flags: superreview?(roc)
Attachment #366215 - Flags: review?(roc)
Attachment #366082 - Flags: superreview?(roc)
Attachment #366082 - Flags: review?(roc)
Attachment #366215 - Flags: superreview?(roc)
Attachment #366215 - Flags: superreview+
Attachment #366215 - Flags: review?(roc)
Attachment #366215 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Changing Component to General for the cases identified by Jonas in bug 474369 comment 60:
> Do you even need this Clear() call? Or even better, could you make this into
> 
> nsAutoTArray<olState, 8> mOLStateStack;
> 
> That would avoid all manual memory management. Feel free to leave this for
> later

> I think you can make mNameSpaces into an nsTArray<nsNameSpaceEntry> to avoid
> the manual memory management. Feel free to leave this for later

> Seems like mObservers should be an nsCOMArray<nsIWeakReference>, that will
> avoid the manual refcounting. Feel free to leave this for later

> You can make this into nsTArray<SortKey> instead. Feel free to leave this for
> later

> This one could be converted to nsTArray<nsTemplateRule> too, though it's quite
> a bit of work so probably better for a separate patch.
Component: Widget → General
QA Contact: general → general
Comment on attachment 366083 [details] [diff] [review]
widget part, v3 [pushed]

http://hg.mozilla.org/mozilla-central/rev/3ca2646bf687
Attachment #366083 - Attachment description: widget part, v3 → widget part, v3 [pushed]
Comment on attachment 366215 [details] [diff] [review]
layout part, v3

http://hg.mozilla.org/mozilla-central/rev/dac7c3176b33
Attachment #366215 - Attachment description: layout part, v3 → layout part, v3 [pushed]
Keywords: checkin-needed
Whiteboard: [needs landing]
Attachment #366215 - Attachment description: layout part, v3 [pushed] → layout part, v3
Comment on attachment 366215 [details] [diff] [review]
layout part, v3

Backed out due to -ve leaks in DR_FrameTypeInfo:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236740534.1236740722.1036.gz

      |&lt;----------------Class--------------->|&lt;-----Bytes------>|&lt;----------------Objects---------------->|&lt;--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
-  23 DR_FrameTypeInfo                               56        0       33        0 (   16.50 +/-     9.61)        0        0 (    0.00 +/-     0.00)
+  23 DR_FrameTypeInfo                               56 -2147483648       33      -33 (    0.00 +/-     0.00)        0        0 (    0.00 +/-     0.00)
Comment on attachment 366215 [details] [diff] [review]
layout part, v3

>   for (PRInt32 i = 0; i < numEntries; i++) {
>+    DR_FrameTypeInfo& info = mFrameTypeTable.ElementAt(i);
>+    if (info.mType == aFrameType) {
>+      return &mFrameTypeTable.ElementAt(i);
Why not return &info (twice)?

>+    FrameData frameData = mFrames.ElementAt(i);
>+
>+    if (frameData.mFrame == aFrame) {
You only use frameData once, so you could write
if (mFrames.ElementAt[i].mFrame == aFrame) {

[If you want to be really lazy, write a comparator for FrameData:
template<class FrameData, class nsIFrame*>
class nsDefaultComparator {
  public:
    PRBool Equals(const FrameData& a, const nsIFrame* b) {
      return a.mFrame == b;
    }
};
and use mFrames.RemoveElement(aFrame); ]

>+    FrameData frameData = mFrames.ElementAt(i);
FrameData& frameData?

>-  DeepTreeStackItem()  { MOZ_COUNT_CTOR(DeepTreeStackItem); }
>-  ~DeepTreeStackItem() { MOZ_COUNT_DTOR(DeepTreeStackItem); }
>+  DeepTreeStackItem()  { MOZ_COUNT_CTOR(DeepTreeStackItem); }
>+  ~DeepTreeStackItem() { MOZ_COUNT_DTOR(DeepTreeStackItem); }
Do we still need to count CTOR/DTOR here or for DR_FrameTypeInfo?
> Do we still need to count CTOR/DTOR here or for DR_FrameTypeInfo?

Indeed, when not doing manual memory management, there is no need for that. This also fixes the negative leak without implementing a copy constructor (the default copy constructor works now).

I also integrated the other comments.
Attachment #366215 - Attachment is obsolete: true
Attachment #366831 - Flags: superreview?(roc)
Attachment #366831 - Flags: review?(roc)
Attachment #366831 - Flags: superreview?(roc)
Attachment #366831 - Flags: superreview+
Attachment #366831 - Flags: review?(roc)
Attachment #366831 - Flags: review+
Comment on attachment 366831 [details] [diff] [review]
layout part, v4 [pushed, comment 17]

http://hg.mozilla.org/mozilla-central/rev/9c36304c687f
Attachment #366831 - Attachment description: layout part, v4 → layout part, v4 [pushed, comment 17]
Attached patch content part (obsolete) — Splinter Review
Based on top of attachment 366833 [details] [diff] [review].
SortKey caused me quite a headache, but I managed to work around nsTArrays copy construction behavior.

> Seems like mObservers should be an nsCOMArray<nsIWeakReference>, that will
> avoid the manual refcounting. Feel free to leave this for later

I rather made this a nsTArray<nsCOMPtr<...> >
Attachment #367401 - Flags: superreview?(jonas)
Attachment #367401 - Flags: review?(jonas)
Attached patch content part, v2 (obsolete) — Splinter Review
The mac boxes had problems with the template specialization. This should work I hope.
Attachment #367401 - Attachment is obsolete: true
Attachment #369242 - Flags: superreview?(jonas)
Attachment #369242 - Flags: review?(jonas)
Attachment #367401 - Flags: superreview?(jonas)
Attachment #367401 - Flags: review?(jonas)
Comment on attachment 369242 [details] [diff] [review]
content part, v2

Some random drive-by comments:

>@@ -337,14 +331,12 @@ nsXMLContentSerializer::PushNameSpaceDec
>                                           const nsAString& aURI,
>                                           nsIDOMElement* aOwner)
> {
>-  NameSpaceDecl* decl = new NameSpaceDecl();
>-  if (!decl) return NS_ERROR_OUT_OF_MEMORY;
>-
>-  decl->mPrefix.Assign(aPrefix);
>-  decl->mURI.Assign(aURI);
>+  NameSpaceDecl decl;
>+  decl.mPrefix.Assign(aPrefix);
>+  decl.mURI.Assign(aURI);
>   // Don't addref - this weak reference will be removed when
>   // we pop the stack
>-  decl->mOwner = aOwner;
>+  decl.mOwner = aOwner;
> 
>   mNameSpaceStack.AppendElement(decl);

You could avoid the copy constructor (and keep the out-of-memory check):

  NameSpaceDecl* decl = mNameSpaceStack.AppendElement();
  if (!decl) return NS_ERROR_OUT_OF_MEMORY;

  decl->mPrefix.Assign(aPrefix);
  ...

>diff --git a/content/base/src/nsXMLNameSpaceMap.cpp b/content/base/src/nsXMLNameSpaceMap.cpp

>+template<>

You should probably use NS_SPECIALIZE_TEMPLATE.

> nsXMLNameSpaceMap::AddPrefix(nsIAtom *aPrefix, PRInt32 aNameSpaceID)

>+  if (!mNameSpaces.Contains(aPrefix)) {
>+    if (!mNameSpaces.AppendElement(aPrefix))

Combine these with &&

>diff --git a/content/svg/content/src/nsSVGValue.h b/content/svg/content/src/nsSVGValue.h

>+  nsAutoTArray<nsCOMPtr<nsIWeakReference>, 1> mObservers;

Maybe nsAutoTArray<nsWeakPtr, 1>?
Attached patch content part, v3 (obsolete) — Splinter Review
With Peters comments fixed.
Attachment #369242 - Attachment is obsolete: true
Attachment #369317 - Flags: superreview?(jonas)
Attachment #369317 - Flags: review?(jonas)
Attachment #369242 - Flags: superreview?(jonas)
Attachment #369242 - Flags: review?(jonas)
Comment on attachment 369317 [details] [diff] [review]
content part, v3

>diff --git a/content/svg/content/src/nsSVGValue.cpp b/content/svg/content/src/nsSVGValue.cpp
>@@ -99,7 +92,7 @@ NS_IMETHODIMP
> NS_IMETHODIMP
> nsSVGValue::AddObserver(nsISVGValueObserver* observer)
> {
>-  nsIWeakReference* wr = NS_GetWeakReference(observer);
>+  nsCOMPtr<nsIWeakReference> wr = do_GetWeakReference(observer);

Use nsWeakRef instead.


>diff --git a/content/xul/templates/src/nsTemplateRule.cpp b/content/xul/templates/src/nsTemplateRule.cpp
>--- a/content/xul/templates/src/nsTemplateRule.cpp
>+++ b/content/xul/templates/src/nsTemplateRule.cpp
>@@ -293,14 +293,11 @@ nsTemplateRule::nsTemplateRule(nsIConten
>           mBindings(nsnull),
>           mConditions(nsnull)
> {
>-    MOZ_COUNT_CTOR(nsTemplateRule);
>     mRuleNode = do_QueryInterface(aRuleNode);
> }
> 
> nsTemplateRule::~nsTemplateRule()
> {
>-    MOZ_COUNT_DTOR(nsTemplateRule);
>-
>     while (mBindings) {
>         Binding* doomed = mBindings;
>         mBindings = mBindings->mNext;

It doesn't hurt to keep these macros. We might end up using this class elsewhere, or we might end up leaking the full array at which point it'd be helpful to know which array is leaking.

>diff --git a/content/xul/templates/src/nsTemplateRule.h b/content/xul/templates/src/nsTemplateRule.h

>+    void RemoveRule(nsTemplateRule *aRule)
>+    {
>+        for (PRUint32 i = 0, n = mRules.Length(); i < n; i++) {
>+            if (&mRules[i] == aRule) {
>+                mRules.RemoveElementAt(i);
>+                break;
>+            }
>+        }
>     }

This should work faster:

PRUint32 index = aRule - mRules.Elements();
if (index < mRules.Length()) {
  mRules.RemoveElementAt(index);
}

You should even be able to do
mRules.RemoveElementAt(aRule - mRules.Elements());
if you're sure the rule actually belongs to the array. Which it looks like you do.

r/sr=me with that fixed.
Attachment #369317 - Flags: superreview?(jonas)
Attachment #369317 - Flags: superreview+
Attachment #369317 - Flags: review?(jonas)
Attachment #369317 - Flags: review+
> It doesn't hurt to keep these macros. We might end up using this class
> elsewhere, or we might end up leaking the full array at which point it'd be
> helpful to know which array is leaking.
That required me to write a copy-constructor. I added a comment that it should only be used by nsTArray.
I will push the patch as soon as the tree reopens.
Attachment #369317 - Attachment is obsolete: true
Attachment #369468 - Flags: superreview+
Attachment #369468 - Flags: review+
Comment on attachment 369468 [details] [diff] [review]
content part, for checkin [pushed: comment 24]

http://hg.mozilla.org/mozilla-central/rev/52c7758d5a4d
Attachment #369468 - Attachment description: content part, for checkin → content part, for checkin [pushed: comment 24]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You can make that copy constructor private and make nsTArrayElementTraits<nsTemplateRule> a friend class to prevent accidental use.
jag: are you doing reviews?
Attachment #369696 - Flags: superreview?(jonas)
Attachment #369696 - Flags: review?(jag)
Attachment #369696 - Flags: review?(jag) → review+
Comment on attachment 369696 [details] [diff] [review]
make the copy constructor private

Try server is generating the following error. Works on my machine however. Don't quite now what should be wrong there.
nsTemplateRule.h:334 is the AppendElement(nsTemplateRule())
/builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/content/xul/templates/src/nsTemplateRule.h:237: error: ‘nsTemplateRule::nsTemplateRule(const nsTemplateRule&)’ is protected
/builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/content/xul/templates/src/nsTemplateRule.h:334: error: within this context
Attachment #369696 - Attachment is obsolete: true
Attachment #369696 - Flags: superreview?(jonas)
Hrm, that's odd. It compiles with gcc 4.0.1 on Mac and gcc 4.1.2 on Linux. It could be the rules have been tightened in gcc and so that even when binding to a const Foo& you now need access to Foo's copy constructor.

Try adding friend class nsTArray<nsTemplateRule> to your previous patch.
(In reply to comment #28)
> Try adding friend class nsTArray<nsTemplateRule> to your previous patch.

Tryserver is still failing with a compile error, whereas with gcc 4.3 on my machine it works fine.
Hrm, I wonder what the tryserver is compiling with.
After downloading a build and checking buildconfig.html:

gcc version 4.1.2 20061011 (Red Hat 4.1.1-29)

I tested using gcc version 4.1.2 20070626 (Red Hat 4.1.2-14)

I wonder what's different.
Attachment #373486 - Flags: superreview?(roc)
Attachment #373486 - Flags: superreview+
Attachment #373486 - Flags: review?(roc)
Attachment #373486 - Flags: review+
Comment on attachment 373486 [details] [diff] [review]
(Ev1) Remove leftover 'i'
[Checkin: Comment 33]


http://hg.mozilla.org/mozilla-central/rev/9c4ad3fbb218
Attachment #373486 - Attachment description: (Ev1) Remove leftover 'i' → (Ev1) Remove leftover 'i' [Checkin: Comment 33]
You need to log in before you can comment on or make changes to this bug.