447838 - Fix warnings in nsTextFrameThebes.cpp

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Steve Snyder]

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

Comment 1

[Steve Snyder]

Attachment: Fixes warnings seen with VS2005/SP1 compiler

Comment 2

[Ed Morley [:emorley]]

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

Comment 3

[Ed Morley [:emorley]]

Attachment: Patch v2

http://dev.philringnalda.com/tbpl/?tree=Try&rev=1148765ecf69

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 5

[Ed Morley [:emorley]]

Attachment: 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 :-)

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Ed Morley [:emorley]]

Attachment: 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?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The cast for NS_lroundup30 is fine.

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

Comment 9

[Ed Morley [:emorley]]

Attachment: 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 :-)

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

float() is no better than (float) really.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #11)
> float() is no better than (float) really.

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

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK.

Comment 15

[Ed Morley [:emorley]]

Attachment: 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

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 544999 [details] [diff] [review]
Patch v6

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

Comment 17

[Ed Morley [:emorley]]

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 :-)

Comment 18

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8c669f508df0

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/8c669f508df0