505181 - Fix signedness warnings in libeditor

Status: RESOLVED WORKSFORME

(Core :: DOM: Editor, defect)

Description

[timeless]

i've had this patch in my tree since for a while, but i guess it was too big and i hadn't decided how to split it. i just noticed that other apps complain about the same code, so it's time to push this fix. credit goes to gcc's compiler for complaining.

Comment 1

[timeless]

Attachment: fixes

Comment 2

[[e-mail]]

Comment on attachment 389436 [details] [diff] [review]
fixes

>-  PropItem *item;
Ah yes, this must have been overlooked by the switch to nsTArray.

>-                                 PRInt32 *aIndex, PRBool aList, PRBool aTbl)
>+                                 PRInt32 &aIndex, PRBool aList, PRBool aTbl)
If anything I think this change makes it less clear that GetInnerContent modifies its parameter. Unhelpfully this modification is currently only needed by its recursive call, although you could in theory rewrite the callers to loop upwards in which case they would want to know the new index.

>-  PRInt32 rangeCount = inArrayOfRanges.Count();
>-  
>-  PRInt32 i;
>+  PRUint32 rangeCount = inArrayOfRanges.Count();
>+  
>+  PRUint32 i;
The rangeCount change I can understand. The i change is less useful...

>-  if (countU < 0 || (PRUint32)countSS != countU)
Nice ;-)

>+    PRInt32 fragLength = textFrag->GetLength();
These changes look reasonable.

>-    PRInt32 mOffset;                      // the offset passed to our contructor
>+    PRInt16 mOffset;                      // the offset passed to our contructor
This looks wrong. If anything, the other offset should be 32-bit.

Comment 3

[timeless]

Attachment: more changes

so, this doesn't take into account neil's comments.

the reason this bug wasn't filed a while ago was that i needed to ask brade or someone about some things, mostly the things neil asked about, especially what the design goal of those 16bit arguments and members was.

the reason i went with the & was to help lame optimizers. i suspect that a PGO optimizer wouldn't care one way or the other

Comment 4

[Kathleen :Brade]

Comment on attachment 389436 [details] [diff] [review]
fixes

Sorry; I'm not comfortable with these changes.  How much testing have you done with these changes?

My intuition says that this is the wrong approach for fixing the signedness warnings.  Instead, I'd poke at nsIPlaintextEditor's "textLength" and "maxTextLength" and go from there.  Note that "textLength" has a comment about it that says:
>     * XXX change this type to 'unsigned long'

Comment 5

[Kathleen :Brade]

Comment on attachment 389448 [details] [diff] [review]
more changes

Also, I don't like the changes to the for loops.  I think it makes it harder for people to understand what the code is doing (and I see very little benefit to making those changes).

Comment 6

[timeless]

Attachment: with changes to nsIPlaintextEditor.textLength and nsWSRunObject.mOffset

This only addresses the following warnings

editor/txtsvc/src/nsTextServicesDocument.cpp:
 In static member function ‘static nsresult nsTextServicesDocument::FindWordBounds(nsTArray<OffsetEntry*>*, nsString*, nsIDOMNode*, PRInt32, nsIDOMNode**, PRInt32*, nsIDOMNode**, PRInt32*)’:
4045: warning: comparison between signed and unsigned integer expressions
4045: warning: comparison between signed and unsigned integer expressions
4068: warning: comparison between signed and unsigned integer expressions
4070: warning: comparison between signed and unsigned integer expressions

editor/libeditor/text/nsTextEditRules.cpp:
 In member function ‘nsresult nsTextEditRules::WillInsertText(PRInt32, nsISelection*, PRBool*, PRBool*, const nsAString_internal*, nsAString_internal*, PRInt32)’:
897: warning: comparison between signed and unsigned integer expressions
 In member function ‘nsresult nsTextEditRules::WillDeleteSelection(nsISelection*, short int, PRBool*, PRBool*)’:
1031: warning: comparison between signed and unsigned integer expressions
 In member function ‘virtual nsresult nsTextEditRules::Notify(nsITimer*)’:
1450: warning: comparison between signed and unsigned integer expressions

editor/libeditor/html/nsHTMLEditRules.cpp:
 In member function ‘nsresult nsHTMLEditRules::GetNodesForOperation(nsCOMArray<nsIDOMRange>&, nsCOMArray<nsIDOMNode>&, PRInt32, PRBool)’:
5834: warning: comparison between signed and unsigned integer expressions

editor/libeditor/html/nsWSRunObject.cpp:
 In member function ‘nsresult nsWSRunObject::GetWSNodes()’:
736: warning: comparison between signed and unsigned integer expressions
804: warning: comparison between signed and unsigned integer expressions
863: warning: comparison between signed and unsigned integer expressions
866: warning: comparison between signed and unsigned integer expressions
869: warning: comparison between signed and unsigned integer expressions
935: warning: comparison between signed and unsigned integer expressions
938: warning: comparison between signed and unsigned integer expressions
 In member function ‘nsresult nsWSRunObject::DeleteChars(nsIDOMNode*, PRInt32, nsIDOMNode*, PRInt32, nsWSRunObject::AreaRestriction)’:
1590: warning: comparison between signed and unsigned integer expressions
 In member function ‘nsresult nsWSRunObject::GetCharAfter(nsWSRunObject::WSPoint&, nsWSRunObject::WSPoint*)’:
1698: warning: comparison between signed and unsigned integer expressions
 In member function ‘PRUnichar nsWSRunObject::GetCharAt(nsIContent*, PRInt32)’:
1944: warning: comparison between signed and unsigned integer expressions

Comment 7

[timeless]

Attachment: with j actually signed

Comment 8

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 445934 [details] [diff] [review]
with j actually signed

nsHTMLEditRules::GetNodesForOperation() also needs changes in lines 5897, 5916, 5932 and 5956...

nsWSRunObject::GetWSNodes() in line 924 and 926, len is a PRUint32, care to test (!len) instead of (len < 1) ?

Comment 9

[Atul Aggarwal]

I could not find above mentioned warnings in editor/ anymore and hence closing this bug.