Closed Bug 505181 Opened 11 years ago Closed 8 years ago

Fix signedness warnings in libeditor

Categories

(Core :: DOM: Editor, defect, minor)

x86
macOS
defect
Not set
minor

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: timeless, Assigned: timeless)

References

(Blocks 1 open bug)

Details

(Whiteboard: [build_warning])

Attachments

(1 file, 3 obsolete files)

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.
Summary: fixed signedness warnings in editor → fixed signedness warnings in libeditor
Summary: fixed signedness warnings in libeditor → Fix signedness warnings in libeditor
Attached patch fixes (obsolete) — Splinter Review
Attachment #389436 - Flags: review?(neil)
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.
Attachment #389436 - Flags: review?(neil) → review-
Attached patch more changes (obsolete) — Splinter Review
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
Attachment #389436 - Attachment is obsolete: true
Attachment #389448 - Flags: superreview?(neil)
Attachment #389448 - Flags: review?(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'
Attachment #389436 - Flags: review-
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).
Attachment #389448 - Flags: review?(brade) → review-
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
Attachment #389448 - Attachment is obsolete: true
Attachment #445920 - Flags: review?(daniel)
Attachment #389448 - Flags: superreview?(neil)
Attachment #445920 - Attachment is obsolete: true
Attachment #445934 - Flags: review?(daniel)
Attachment #445920 - Flags: review?(daniel)
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) ?
Attachment #445934 - Flags: review?(daniel) → review-
Blocks: buildwarning
Whiteboard: [build_warning]
I could not find above mentioned warnings in editor/ anymore and hence closing this bug.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.