Last Comment Bug 447838 - Fix warnings in nsTextFrameThebes.cpp
: Fix warnings in nsTextFrameThebes.cpp
Status: RESOLVED FIXED
[build_warning]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Ed Morley [:emorley]
:
Mentors:
Depends on: 670111 670338
Blocks: buildwarning
  Show dependency treegraph
 
Reported: 2008-07-24 11:49 PDT by Steve Snyder
Modified: 2011-07-15 08:04 PDT (History)
4 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fixes warnings seen with VS2005/SP1 compiler (6.10 KB, patch)
2008-07-24 11:50 PDT, Steve Snyder
no flags Details | Diff | Splinter Review
Patch v2 (14.53 KB, patch)
2011-07-05 11:55 PDT, Ed Morley [:emorley]
no flags Details | Diff | Splinter Review
Patch v3 (14.71 KB, patch)
2011-07-06 04:26 PDT, Ed Morley [:emorley]
no flags Details | Diff | Splinter Review
Patch v4 (15.60 KB, patch)
2011-07-07 10:50 PDT, Ed Morley [:emorley]
no flags Details | Diff | Splinter Review
Patch v5 (4.95 KB, patch)
2011-07-08 04:56 PDT, Ed Morley [:emorley]
no flags Details | Diff | Splinter Review
Patch v6 (4.98 KB, patch)
2011-07-09 09:22 PDT, Ed Morley [:emorley]
roc: review+
Details | Diff | Splinter Review

Description Steve Snyder 2008-07-24 11:49:11 PDT
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 2008-07-24 11:50:40 PDT
Created attachment 331154 [details] [diff] [review]
Fixes warnings seen with VS2005/SP1 compiler
Comment 2 Ed Morley [:emorley] 2011-07-05 03:15:03 PDT
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 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-05 15:56:54 PDT
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] 2011-07-06 04:26:44 PDT
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 :-)
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-06 14:51:28 PDT
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] 2011-07-07 10:50:24 PDT
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?
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-07 15:46:19 PDT
The cast for NS_lroundup30 is fine.

Looks like NSToCoordFloor, NSToCoordCeil, NSToCoordCeilClamped would also benefit from double overloads :-)
Comment 9 Ed Morley [:emorley] 2011-07-08 04:56:58 PDT
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 :-)
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2011-07-08 05:05:08 PDT
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) 2011-07-08 15:19:45 PDT
float() is no better than (float) really.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-08 15:21:05 PDT
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-7 (review requests must explain patch) 2011-07-08 15:29:06 PDT
(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) 2011-07-08 16:39:17 PDT
OK.
Comment 15 Ed Morley [:emorley] 2011-07-09 09:22:03 PDT
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
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-10 03:25:37 PDT
Comment on attachment 544999 [details] [diff] [review]
Patch v6

Review of attachment 544999 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 17 Ed Morley [:emorley] 2011-07-10 03:50:23 PDT
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 :-)

Note You need to log in before you can comment on or make changes to this bug.