Closed Bug 78388 Opened 19 years ago Closed 7 years ago

Fix uses of |new nsAutoString / nsCAutoString|

Categories

(Core :: String, defect)

defect
Not set

Tracking

()

RESOLVED INCOMPLETE
mozilla1.9.1b1

People

(Reporter: dbaron, Assigned: sgautherie)

References

()

Details

Attachments

(5 files, 5 obsolete files)

We have a bunch of nsAutoString explicitly allocated on the heap (in addition to
the ones that are member variables, see bug 26622):

http://lxr.mozilla.org/seamonkey/search?string=new+nsAutoString

We should fix these.  (They're probably also good places to look for other bad
string usage.)


Once we clean this up, we could also probably do a little funny math and make
nsAutoString assert if it's not allocated on the stack.
scc:  What's our policy here?  Do we want to prevent *all* heap-allocated
nsAutoStrings?  There are a number of places where people use heap-allocated
nsAutoStrings to fuse allocation (at least, I think that was the intent,
although it could have been ignorance) in objects that I think are shortlived.
Priority: -- → P3
Target Milestone: --- → mozilla0.9.3
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Target Milestone: mozilla0.9.7 → Future
QA Contact: scc → string
Summary: Fix uses of |new nsAutoString|. → Fix uses of |new nsAutoString / nsCAutoString|
Attached patch (Bv1) 2 <ns*RuleProcessor.*> (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080819081634 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)
Assignee: dbaron → sgautherie.bz
Attachment #334455 - Flags: superreview?(dbaron)
Attachment #334455 - Flags: review?(dbaron)
Flags: in-testsuite-
Priority: P3 → --
Target Milestone: Future → mozilla1.9.1a2
Flags: in-testsuite- → in-testsuite+
Comment on attachment 334455 [details] [diff] [review]
(Bv1) 2 <ns*RuleProcessor.*>

Given that this should share string buffers, auto strings aren't useful here anymore, so this is fine.

However, you should add an:

NS_ASSERTION(hasAttr || mLanguage.IsEmpty(), "GetAttr that returns false should not make string non-empty"); before the final "if (hasAttr)".

r+sr=dbaron with that
Attachment #334455 - Flags: superreview?(dbaron)
Attachment #334455 - Flags: superreview+
Attachment #334455 - Flags: review?(dbaron)
Attachment #334455 - Flags: review+
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080828170525 Minefield/3.1a2pre] (home, debug default) (W2Ksp4)

Seems to run without triggering the assertion... :-)
Attachment #334455 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [c-n: Bv1a // Leave opened]
Target Milestone: mozilla1.9.1a2 → mozilla1.9.1b1
Attached patch (Cv1) <nsDataObj.cpp> (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080828200904 Minefield/3.1a2pre] (home, debug default) (W2Ksp4)

*|mDataFlavors| items: s/nsCAutoString/nsCString/g.
*Some related code/comment rewrites.
 *s/hangled/handled/

Iiuc, this will add an allocation+deallocation for each flavor.
I assume this D&D path/feature is not (perf) critical so this is acceptable.

I wonder if |mDataFlavors| itself really needs to be a pointer,
or if it could simply be a direct part of the |nsDataObj| class ?
<http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.h>
Attachment #335972 - Flags: superreview?(mats.palmgren)
Attachment #335972 - Flags: review?(emaijala)
Comment on attachment 335972 [details] [diff] [review]
(Cv1) <nsDataObj.cpp>

+  for (PRUint32 idx = mDataEntryList.Length(); idx-- > 0;) {

Í'd place the decrement operation to the third field of the for clause for readability. Other than that, r=me.
Attachment #335972 - Flags: review?(emaijala) → review+
(In reply to comment #7)
> (From update of attachment 335972 [details] [diff] [review])
> +  for (PRUint32 idx = mDataEntryList.Length(); idx-- > 0;) {
> 
> Í'd place the decrement operation to the third field of the for clause for

That wouldn't work.

> readability. Other than that, r=me.

I could move it to the beginning of the loop, if you prefer. (I don't.)
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 335972 [details] [diff] [review] [details])
> > +  for (PRUint32 idx = mDataEntryList.Length(); idx-- > 0;) {
> > 
> > Í'd place the decrement operation to the third field of the for clause for
> 
> That wouldn't work.

Please enlighten me as I'm failing miserably to see why this wouldn't work:

  for (PRUint32 idx = mDataEntryList.Length(); idx > 0; idx--) {
(In reply to comment #9)
>   for (PRUint32 idx = mDataEntryList.Length(); idx > 0; idx--) {

Because |idx| is used inside the loop:
*that would "need" to add a |- 1| to the first field.
*even so, the last iteration, with the |0| value, would not happen.
(In reply to comment #10)
> (In reply to comment #9)
> >   for (PRUint32 idx = mDataEntryList.Length(); idx > 0; idx--) {
> 
> Because |idx| is used inside the loop:
> *that would "need" to add a |- 1| to the first field.
> *even so, the last iteration, with the |0| value, would not happen.

Sorry, I meant to make it:

  for (PRUint32 idx = mDataEntryList.Length() - 1; idx >= 0; idx--) {
Oops, it's unsigned. Never mind.
(In reply to comment #12)
> Oops, it's unsigned. Never mind.

Yeah ;-> Then, is it all right with you to leave it as I wrote it ?
Yeah :)
Comment on attachment 335899 [details] [diff] [review]
(Bv1a) 2 <ns*RuleProcessor.*>
[Checkin: Comment 15]

http://hg.mozilla.org/mozilla-central/rev/837cacb67631
Attachment #335899 - Attachment description: (Bv1a) 2 <ns*RuleProcessor.*> → (Bv1a) 2 <ns*RuleProcessor.*> [Checkin: Comment 15]
Keywords: checkin-needed
Whiteboard: [c-n: Bv1a // Leave opened]
Depends on: 453341
Comment on attachment 335972 [details] [diff] [review]
(Cv1) <nsDataObj.cpp>

> nsDataObj::~nsDataObj()

This method isn't perf sensitive and the arrays are small
so I see no reason to complicate the loops here to save
a few calls to Count()/Length().
I prefer the current code since it's more readable.
Feel free to move the "PRInt32" and add the spaces
in the first loop though.

sr=mats
Attachment #335972 - Flags: superreview?(mats.palmgren) → superreview+
Cv1, with comment 16 suggestion(s) :-|
Attachment #335972 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [c-n: Cv1a // Leave opened]
Comment on attachment 336745 [details] [diff] [review]
(Cv1a) <nsDataObj.cpp>
[Checkin: Comment 18]

http://hg.mozilla.org/mozilla-central/rev/3632d232b2c5
Attachment #336745 - Attachment description: (Cv1a) <nsDataObj.cpp> → (Cv1a) <nsDataObj.cpp> [Checkin: Comment 18]
(In reply to comment #6)
> I wonder if |mDataFlavors| itself really needs to be a pointer,
> or if it could simply be a direct part of the |nsDataObj| class ?
> <http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.h>

Ere, Mats, what about my question ?
Keywords: checkin-needed
Whiteboard: [c-n: Cv1a // Leave opened]
(In reply to comment #19)
> (In reply to comment #6)
> > I wonder if |mDataFlavors| itself really needs to be a pointer,
> > or if it could simply be a direct part of the |nsDataObj| class ?
> > <http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.h>
> 
> Ere, Mats, what about my question ?

Oh, right, sorry about that. I can't see any reason why it should be a pointer.
(In reply to comment #20)
> I can't see any reason why it should be a pointer.

I filed bug 455557.
Uncompiled...
Attachment #340691 - Flags: review?(alfred.peng)
Attached patch (Ev1) <nsWindow.*> (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081012 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)

(compiled; assuming run...)

Ere, Mats: would it be an (unwanted) pref regression to not use an "Auto" string ?

Benjamin: in case the "Auto" is kept, should I do something else than the (3) comment I added ?
Attachment #342826 - Flags: superreview?(mats.palmgren)
Attachment #342826 - Flags: review?(emaijala)
Attachment #342826 - Flags: review?(bsmedberg)
Mike: I assume the "Auto" is/are really wanted as is ?

Benjamin: in case the "Auto" is kept, should I do something else than the (2)
comment I added ?
Attachment #342828 - Flags: superreview?(shaver)
Attachment #342828 - Flags: review?(shaver)
Attachment #342828 - Flags: review?(bsmedberg)
Attachment #340691 - Flags: review?(xiaobin.lu)
(In reply to comment #23)
> Ere, Mats: would it be an (unwanted) pref regression to not use an "Auto"
> string ?

I'm not that good with the string classes, but I can't see the reason for using Auto string. It's not performance critical code though, and perhaps there is a reason for this (some specific use pattern or such) so another opinion would be appreciated.
(In reply to comment #25)
> (In reply to comment #23)
> > Ere, Mats: would it be an (unwanted) pref regression to not use an "Auto"
> > string ?
> 
> I'm not that good with the string classes, but I can't see the reason for using
> Auto string.

http://mxr.mozilla.org/mozilla-central/search?string=sIMECompUnicode&case=on&find=%2Fwidget%2Fsrc%2Fwindows%2FnsWindow%5C

(afaict) The only reason is that there is 2 |Truncate()| and the memory space is reused.

Considering code like
7460     PRInt32 len = sIMECompUnicode->Length();
7461     if (len > IME_MAX_CHAR_POS)
I wonder if |char sIMECompUnicode[64]| would not even do the job !?
(In reply to comment #26)
> I wonder if |char sIMECompUnicode[64]| would not even do the job !?

Or |char sIMECompUnicode[IME_MAX_CHAR_POS + 1]| if it's really a string with terminating \0. I think IME isn't performance critical though, so perhaps it isn't worth it.
Attached patch (Ev1a) <nsWindow.*> (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081012 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)

(compiled, but not run)

NB: I would have done it with "char []", but the string being Unicode would make it "needlessly" more complicated, so I concur with comment 28.

Ftr, ImmGetCompositionString() documentation is at
http://msdn.microsoft.com/en-us/library/ms776186(VS.85).aspx
Attachment #342826 - Attachment is obsolete: true
Attachment #342826 - Flags: superreview?(mats.palmgren)
Attachment #342826 - Flags: review?(emaijala)
Attachment #342826 - Flags: review?(bsmedberg)
Attachment #343411 - Flags: superreview?(mats.palmgren)
Attachment #343411 - Flags: review?(emaijala)
Ev1a, with 2 "nits".
Attachment #343411 - Attachment is obsolete: true
Attachment #343411 - Flags: superreview?(mats.palmgren)
Attachment #343411 - Flags: review?(emaijala)
Attachment #343415 - Flags: superreview?(mats.palmgren)
Attachment #343415 - Flags: review?(emaijala)
Comment on attachment 343415 [details] [diff] [review]
(Ev1b) <nsWindow.*>
[Checkin: See comment 32]

It could have been a wide char array, but I like this better.
Attachment #343415 - Flags: review?(emaijala) → review+
Comment on attachment 343415 [details] [diff] [review]
(Ev1b) <nsWindow.*>
[Checkin: See comment 32]


sr=mats with the following nits fixed:

> -  NS_ASSERTION(sIMECompUnicode, "sIMECompUnicode is null");
> +nsWindow::HandleTextEvent(HIMC hIMEContext, PRBool aCheckAttr)
> +{
> +  if (NS_UNLIKELY(!sIMECompUnicode)) {
> +    NS_ASSERTION(sIMECompUnicode, "sIMECompUnicode is null");
> +    return;
> +  }

I think the current code is more readable and I don't see a compelling
reason to change it.

> nsWindow::GetTextRangeList
> -  NS_ASSERTION(sIMECompUnicode, "sIMECompUnicode is null");

same

> +  if (NS_UNLIKELY(lRtn < 0 ||
> +                  !EnsureStringLength(*sIMECompUnicode, (lRtn / sizeof(WCHAR)) + 1)))

This line is longer than 80 chars. (You can just drop the NS_UNLIKELY,
this code isn't perf sensitive).

> -      PRUint32 i = 0;
> -      for (i = 0; i < sIMECompUnicode->Length(); i++) {
> -        if (PT_IN_RECT(*ptPos, sIMECompCharPos[i]))
> -          break;
> +      PRUint32 i = 0, len = sIMECompUnicode->Length();
> +      while (i < len && !PT_IN_RECT(*ptPos, sIMECompCharPos[i])) {
> +        ++i;

I prefer the old loop construct which I find easier to read.
Feel free to lift out the Length() call to a separate line if you want:
  PRUint32 len = sIMECompUnicode->Length();
but 'len' is guaranteed to be <= IME_MAX_CHAR_POS (64) at this point
so I don't think it's a perf problem to leave it as is.
Attachment #343415 - Flags: superreview?(mats.palmgren) → superreview+
Comment on attachment 343415 [details] [diff] [review]
(Ev1b) <nsWindow.*>
[Checkin: See comment 32]

http://hg.mozilla.org/mozilla-central/rev/039a107a3cec
with comment 31 suggestion(s).
Attachment #343415 - Attachment description: (Ev1b) <nsWindow.*> → (Ev1b) <nsWindow.*> [Checkin: See comment 32]
Attachment #340691 - Attachment description: (Dv1) <ojiapitests.h> → (Dv1) <ojiapitests.h> [Superseded by bug 485984]
Attachment #340691 - Attachment is obsolete: true
Attachment #340691 - Flags: review?(xiaobin.lu)
Attachment #340691 - Flags: review?(alfred.peng)
Depends on: 485984
Attachment #342828 - Flags: review?(bsmedberg) → review?(benjamin)
Attachment #342828 - Flags: review?(benjamin)
Comment on attachment 342828 [details] [diff] [review]
(Fv1) <xpcwrappednative.cpp>

This is rotted, but the comment in the first chunk seems vacuous (it duplicates the content of the preceding and more detailed comment) and the second one doesn't mean anything to me, so it's probably not clear to others reading the code either!
Attachment #342828 - Flags: superreview?(shaver)
Attachment #342828 - Flags: review?(shaver)
Attachment #342828 - Flags: review-
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.