Fix warnings in nsTextFrameThebes.cpp

RESOLVED FIXED in mozilla8

Status

()

Core
Layout
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Steve Snyder, Assigned: emorley)

Tracking

(Blocks: 1 bug)

Trunk
mozilla8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

9 years ago
While doing a Win32/PGO (static) build of SeaMonkey, the final relinking step aborted with an internal compiler (VS2005/SP1) error in file nsTextFrameThebes.cpp.  Not knowing what else to do, I fixed the many compiler warnings seen in the compilation of this source files, erased the objdir, then rebuilt.  The build completed successfully.

The patch that follows fixes the warnings shown below.  It is intended as a work-around to an apparent bug in Microsoft's compiler/linker.

I didn't take the time to narrow down which warning(s) were responsible for VS2005's problem.  Maybe it's that single "unsafe operation", or maybe it is one or more of the type conversion warnings.  All I know is that with the original file I could not complete the build, whereas I can with the warnings fixed.  The original nsTextFrameThebes.cpp is from the 22 Jul 2008 trunk.

YMMV.

-------------------------

c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(2326) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(4533) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5022) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5035) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5111) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5123) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5135) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5185) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5186) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5187) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5188) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5303) : warning C4806: '&' : unsafe operation: no value of type 'bool' promoted to type 'int' can equal the given constant
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5598) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5605) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5606) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5615) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5615) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5616) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5616) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5642) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
c:/mozbuild/mozilla/layout/generic/nsTextFrameThebes.cpp(5810) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
(Reporter)

Comment 1

9 years ago
Created attachment 331154 [details] [diff] [review]
Fixes warnings seen with VS2005/SP1 compiler
Assignee: general → swsnyder
Status: NEW → ASSIGNED
Assignee: swsnyder → nobody
Component: General → Layout
Product: SeaMonkey → Core
QA Contact: general → layout
Whiteboard: [patchlove]
Assignee: nobody → swsnyder
(Assignee)

Updated

6 years ago
Assignee: swsnyder → bmo
OS: Windows 2000 → Windows 7
(Assignee)

Comment 2

6 years ago
Win debug MSVC2005 (from try log), tip:

nsTextFrameThebes.cpp(136) : warning C4244: 'initializing' : conversion from 'PRUint32' to 'float', possible loss of data
nsTextFrameThebes.cpp(2755) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(2834) : warning C4244: 'argument' : conversion from 'double' to 'PRUint32', possible loss of data
nsTextFrameThebes.cpp(4421) : warning C4244: 'initializing' : conversion from 'const gfxFloat' to 'nscoord', possible loss of data
nsTextFrameThebes.cpp(4802) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'nscoord', possible loss of data
nsTextFrameThebes.cpp(4802) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'nscoord', possible loss of data
nsTextFrameThebes.cpp(4802) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'nscoord', possible loss of data
nsTextFrameThebes.cpp(4802) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'nscoord', possible loss of data
nsTextFrameThebes.cpp(5201) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(5223) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(5680) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(6273) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(6286) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(6310) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'nscoord', possible loss of data
nsTextFrameThebes.cpp(6408) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(6420) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(6496) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(6497) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(6498) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(6499) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(7064) : warning C4244: 'argument' : conversion from 'const double' to 'float', possible loss of data
nsTextFrameThebes.cpp(7071) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(7072) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(7080) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(7081) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(7107) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(7306) : warning C4244: 'argument' : conversion from 'gfxFloat' to 'float', possible loss of data
nsTextFrameThebes.cpp(7482) : warning C4244: 'argument' : conversion from 'PRUnichar' to 'nsACString_internal::char_type', possible loss of data
(Assignee)

Updated

6 years ago
Whiteboard: [patchlove] → [build_warning] [patchlove]
(Assignee)

Updated

6 years ago
Blocks: 187528
(Assignee)

Comment 3

6 years ago
Created attachment 544013 [details] [diff] [review]
Patch v2

http://dev.philringnalda.com/tbpl/?tree=Try&rev=1148765ecf69
Attachment #331154 - Attachment is obsolete: true
Attachment #544013 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Whiteboard: [build_warning] [patchlove] → [build_warning]
Comment on attachment 544013 [details] [diff] [review]
Patch v2

Review of attachment 544013 [details] [diff] [review]:
-----------------------------------------------------------------

The float casts are OK, but when going back to integers or nscoords, please explicitly use NSToCoordRound or NSToIntRound.
(Assignee)

Comment 5

6 years ago
Created attachment 544191 [details] [diff] [review]
Patch v3

> The float casts are OK, but when going back to integers or nscoords, please
> explicitly use NSToCoordRound or NSToIntRound.

Done. However, this meant adding NSToCoordRound((float)foo) etc in some of the instances, otherwise build warnings remained. Some of the casts are looking quite messy now, but at the build warnings are all gone at least. Also not sure whether (float)foo or float(foo) is preferred throughout. 

Thanks :-)
Attachment #544013 - Attachment is obsolete: true
Attachment #544191 - Flags: review?(roc)
Attachment #544013 - Flags: review?(roc)
I think we should add a 'double' overload to NSToCoordRound so that we don't need those float casts, which are just slow and information-losing.
(Assignee)

Comment 7

6 years ago
Created attachment 544543 [details] [diff] [review]
Patch v4

> I think we should add a 'double' overload to NSToCoordRound so that we don't
> need those float casts, which are just slow and information-losing.

Done :-)
Though I had to add a float cast to the win32 path in the new NSToCoordRound double overload, to prevent another warning - since there isn't a double version of NS_lroundup30 (http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsMathUtils.h#71).

Any ideas?
Attachment #544191 - Attachment is obsolete: true
Attachment #544543 - Flags: feedback?(roc)
Attachment #544191 - Flags: review?(roc)
The cast for NS_lroundup30 is fine.

Looks like NSToCoordFloor, NSToCoordCeil, NSToCoordCeilClamped would also benefit from double overloads :-)
(Assignee)

Updated

6 years ago
Depends on: 670111
(Assignee)

Comment 9

6 years ago
Created attachment 544781 [details] [diff] [review]
Patch v5

I've broken out the addition of the double overloads of NSToCoord(Round|Floor|Ceil|Clamped) into bug 670111. There are actually now only a few changes needed to nsTextFrameThebes.cpp to stop all the build warnings (once bug 670111 lands), since most of the previous changes were just float casts. Looks a lot tidier now :-)
Attachment #544543 - Attachment is obsolete: true
Attachment #544781 - Flags: review?(roc)
Attachment #544543 - Flags: feedback?(roc)
Comment on attachment 544781 [details] [diff] [review]
Patch v5

>--- a/layout/generic/nsTextFrameThebes.cpp
>+++ b/layout/generic/nsTextFrameThebes.cpp
> struct TabWidth {
>   TabWidth(PRUint32 aOffset, PRUint32 aWidth)
>-    : mOffset(aOffset), mWidth(aWidth)
>+    : mOffset(aOffset), mWidth((float) aWidth)

And float(aWidth) (or static_cast<float>(aWidth)) would be a little tidier still.
float() is no better than (float) really.
Comment on attachment 544781 [details] [diff] [review]
Patch v5

Review of attachment 544781 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrameThebes.cpp
@@ +2831,5 @@
>          double nextTab = AdvanceToNextTab(mOffsetFromBlockOriginForTabs,
>                  mFrame, mTextRun, &tabWidth);
>          mTabWidths->mWidths.AppendElement(
> +          TabWidth(i - startOffset, 
> +                   NSToIntRound(float(nextTab - mOffsetFromBlockOriginForTabs))));

So we still need a double overload for NSToIntRound?
(In reply to comment #11)
> float() is no better than (float) really.

float() has much more obvious operator precedence; I prefer it.
OK.
(Assignee)

Updated

6 years ago
Depends on: 670338
(Assignee)

Comment 15

6 years ago
Created attachment 544999 [details] [diff] [review]
Patch v6

> So we still need a double overload for NSToIntRound?
Filed bug 670338.

Updated patch with the NSToIntRound float cast removed thanks to bug 670338 & the (float) vs float() style change from comment 10.

http://dev.philringnalda.com/tbpl/?tree=Try&rev=24df92bc84bb
Attachment #544781 - Attachment is obsolete: true
Attachment #544999 - Flags: review?(roc)
Attachment #544781 - Flags: review?(roc)
Comment on attachment 544999 [details] [diff] [review]
Patch v6

Review of attachment 544999 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #544999 - Flags: review?(roc) → review+
(Assignee)

Comment 17

6 years ago
Passed try (comment 15). 

Will result in some build warnings left until bug 670111 and bug 670338 land - though they are both checkin-needed as well, so hopefully someone will push all at once :-)
Keywords: checkin-needed
OS: Windows 7 → All
Hardware: x86 → All
http://hg.mozilla.org/integration/mozilla-inbound/rev/8c669f508df0
Flags: in-testsuite-
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/8c669f508df0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [build_warning]
Target Milestone: --- → mozilla8
(Assignee)

Updated

6 years ago
Whiteboard: [build_warning]
You need to log in before you can comment on or make changes to this bug.