there's no need for mailnews to invent own data structures when xpcom already provides them.
15 years ago
Depends on: 251277
Assignee: cbiesinger → cst
Created attachment 157774 [details] [diff] [review] preliminary patch <CTho> ###!!! ASSERTION: Lossy value array detected. Report a higher maximum upon construction!: '*((PRUint8*)&mValueArray[aIndex * mBytesPerValue]) == aValue', file <CTho> d:/cvs-1.11.5/mozilla/xpcom/ds/nsValueArray.cpp, line 191 <bz> CTho: er... what's aValue there? <CTho> aValue = 258 <bz> CTho: and this was being stored in a PRUint8? <CTho> apparently <bz> CTho: smoooth The caller: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/src/nsMsgDBView.cpp&mark=4486-4490#4486 bienvenu, neil, mscott are apparently guilty for this part of the code
We should really add an easy way to SetValueAt (or fix operator to allow it on the LHS) too for nsValueArray.
Status: NEW → ASSIGNED
Created attachment 157775 [details] Bustage likely caused by my patch I don't use threads normally so this may occur normally, but I doubt it. Most subjects appear blank in the list. Clicking the down-arrow next to a subject produces: ###!!! ASSERTION: couldn't find thread: 'PR_FALSE', file d:/cvs-1.11.5/mozilla/mailnews/base/src/nsMsgDBView.cpp, line 3928
Allowing values up to 1024 got rid of the aValue asserts, but didn't solve the problem in the attached screenshot.
OK, the most likely cause of your issue is that InsertAt's parameters are (position, value) but InsertValueAt is expecting (value, position). nsValueArray could really do with a ReplaceValueAt method. nsMsgDBView::ReverseThreads doesn't need to allocate its arrays on the heap i.e. use nsValueArray newLevelArray(255, m_levels.Count()); and change the ->s to .s nsMsgDBView::ReverseThreads uses a loop to copy a portion of one array to another array which is a bit inefficient but offhand I can't see a better way.
Created attachment 157891 [details] [diff] [review] Patch to remove nsUint8Array Neil, thanks - that fixed it.
Created attachment 157892 [details] [diff] [review] Patch to remove nsUint8Array trivial changes
Attachment #157891 - Attachment is obsolete: true
Comment on attachment 157892 [details] [diff] [review] Patch to remove nsUint8Array >+ :m_levels(255,0) You don't need the 0 as it's the default value for that parameter. >- nsUint8Array *newLevelArray = new nsUint8Array; >+ nsValueArray *newLevelArray = new nsValueArray(255,0); The second parameter needs to be m_levels.GetSize() to preallocate. Also this variable could be converted from a pointer to a local i.e. allocated on the stack rather than on the heap. >+ //XXX cst - we need a SetValueAt for nsValueArray Maybe you should get that fixed first? >- while ((int32) threadIndex < m_levels.GetSize() && m_levels[threadIndex] != 0); >+ while ((int32) threadIndex < m_levels.Count() && m_levels[threadIndex] != 0); Count() returns an unsigned while for some odd reason GetSize() returns a signed, so you'll have to fix up some signed/unsigned mismatches. Hopefully this will be a case of removing erroneous casts :-) >+ m_levels.AppendValue(PRUint8(levelToAdd)); You don't need these casts. >+ m_prevLevels.Clear(); >+ m_prevLevels = m_levels; You don't need to clear before copy. >- m_levels.SetAtGrow(index, 0); You didn't replace this line.
Attachment #157892 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Depends on: 258058
(In reply to comment #8) > >+ //XXX cst - we need a SetValueAt for nsValueArray > Maybe you should get that fixed first? ok, see bug blocking this one > >- m_levels.SetAtGrow(index, 0); > You didn't replace this line. nsValueArray doesn't seem to have an equivalent. It's a performance thing, not a functionality thing, right?
it's a performance thing, but we don't want to reduce performance.
SetAtGrow allows the array to grow, SetAt does not. nsValueArray doesn't have an equivalent, but your new ReplaceValueAt should be able to handle it.
Comment on attachment 157892 [details] [diff] [review] Patch to remove nsUint8Array >- newLevelArray->SetSize(m_levels.GetSize()); Actually what I said last time I commented on the patch was wrong in a different way, but this code expects to be able to preallocate space so that it can be set later. >- m_levels.SetSize(numThreads); And here the code expects to be able to change the count too. >- m_levels.SetAtGrow(index, 0); This would seem to be a false alarm - index is always valid, so it's the same as SetAt.
Depends on: 258098
Whiteboard: blocked → helpwanted
Status: ASSIGNED → NEW
Assignee: cst → sspitzer
Target Milestone: --- → Future
sorry for the spam. making bugzilla reflect reality as I'm not working on these bugs. filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Summary: replace nsUint8Array with nsValueArray → replace nsUint8Array with nsTArray<PRUint8>
Created attachment 295013 [details] [diff] [review] Proposed patch I changed newLevelArray to be stack-allocated instead of heap-allocated (the actual array elements are still heap-allocated) and swapped instead of copying. CollapseByIndex now removes all the elements at once. I tweaked nsMsgGroupView::OnNewHeader, let me know if you don't like it. This even fixes a signed/unsigned mismatch warning :-)
Oh, and I decided that the SetAtGrow wasn't necessary as there must already be a key for that index.
Created attachment 295377 [details] [diff] [review] Updated for bitrot I also accidentally deleted a hunk from a Makefile.in in the last patch.
Comment on attachment 295377 [details] [diff] [review] Updated for bitrot cool, thx, Neil.
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.9 M11
You need to log in before you can comment on or make changes to this bug.