595756 - Fix build warning - 'argument' : conversion from 'PRUint16' to 'PRUint8', possible loss of data

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Serge Gautherie (:sgautherie)]

(I hadn't tried to build locally for a long time...)

After applying bug 595708 comment 3 workaround, I get
{
.../DOMSVGLength.cpp(58) : error C2248: 'mozilla::DOMSVGLengthList::mItems' : cannot access private member declared in class 'mozilla::DOMSVGLengthList'
        ...\DOMSVGLengthList.h(150) : see declaration of 'mozilla::DOMSVGLengthList::mItems'
        ...\DOMSVGLengthList.h(70) : see declaration of 'mozilla::DOMSVGLengthList'

.../DOMSVGLength.cpp(276) : warning C4244: 'argument' : conversion from 'PRUint16' to 'PRUint8', possible loss of data
}

http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/DOMSVGLength.cpp
56   // We may not belong to a list, so we must null check tmp->mList.
57   if (tmp->mList) {
58     tmp->mList->mItems[tmp->mListIndex] = nsnull;

Comment 1

[Serge Gautherie (:sgautherie)]

Iiuc, it looks like VC++ 2005 has difficulty w.r.t.
{
class DOMSVGLengthList : public nsIDOMSVGLengthList
{
  friend class DOMSVGLength;
}

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Add an unrelated PRUint8() cast [Checked in: Comment 26]

This is like
285   mUnit = PRUint8(aUnit);

I have no idea whether the API is right, but at least this patch is consistent with current code.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 477529 [details] [diff] [review]
(Av1) Add an unrelated PRUint8() cast
[Checked in: Comment 26]

"approval2.0=?":
Trivial compile warning fix, zero risk.

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 477529 [details] [diff] [review]
(Av1) Add an unrelated PRUint8() cast
[Checked in: Comment 26]

http://hg.mozilla.org/mozilla-central/rev/2d184fdf2776

Comment 5

[Daniel Holbert [:dholbert]]

Looks like this never got approval, which current tree rules say it's supposed to have before landing.

(having said that, it's trivial & probably merits approval, & you might be able to get retroactive approval by pinging a release driver)

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)

> Looks like this never got approval, which current tree rules say it's supposed
> to have before landing.

Not my fault:
the tree was green-'OPEN' when I checked in, (just as it is "still" green-'APPROVAL REQUIRED' instead of usual yellow).
Moreover, as you can see, I didn't (have to) use 'a=*'.

> (having said that, it's trivial & probably merits approval, & you might be able
> to get retroactive approval by pinging a release driver)

Feel free to just set the flag(s) again:
I have tried to ping for approval both on bugzilla and on irc before, without success...

Comment 7

[:Gavin Sharp [email: [e-mail]]]

I backed out all the changesets in your push because they weren't approved.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> Looks like this never got approval, which current tree rules say it's supposed
> to have before landing.

Oh! I just checked
https://wiki.mozilla.org/Tree_Rules#mozilla-central_-_Trunk_.28Firefox_4.0.2C_Gecko_2.0_work.29
which indeed reads
"When the tree is marked OPEN, you are free to land approved patches after checking with the current sheriff."

Yet, this is quite unexpected/confusing:
'OPEN' (on the waterfall) usually means "no restriction";
"approval needed" usually means there is a checking hook and the waterfall reads 'APPROVAL REQUIRED' :-/

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 477529 [details] [diff] [review]
(Av1) Add an unrelated PRUint8() cast
[Checked in: Comment 26]

(In reply to comment #7)
> I backed out all the changesets in your push because they weren't approved.

See discussion in previous comments :-<

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> Yet, this is quite unexpected/confusing:
> 'OPEN' (on the waterfall) usually means "no restriction";
> "approval needed" usually means there is a checking hook and the waterfall
> reads 'APPROVAL REQUIRED' :-/

(Yup -- it was a mistake that the waterfall said "open", and it's now been updated)

Comment 11

[Serge Gautherie (:sgautherie)]

"blocking2.0=?":
Can't build on tier-1 platform.

Comment 12

[Robert Longson [:longsonr]]

I use VC 2005 Express and everything builds fine for me. The patch is just a cosmetic change and should not affect whether things compile or not.

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)

> I use VC 2005 Express and everything builds fine for me.

Ah. Could it be related to PSDK v2003R2 then?

> The patch is just a
> cosmetic change and should not affect whether things compile or not.

It doesn't and is labelled "unrelated".

Comment 14

[Robert Longson [:longsonr]]

I have Microsoft Platform SDK for Windows Server 2003 R2 installed.

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> I use VC 2005 Express

Ah, do you have SP1? (I don't.)

NB: I'm now wondering whether non-SP1 may be starting to have difficulties with our more recent code. I'll try to upgrade...

(In reply to comment #14)
> I have Microsoft Platform SDK for Windows Server 2003 R2 installed.

Are you building with it only? (as I do on my Win2K)

Comment 16

[Robert Longson [:longsonr]]

Microsoft Visual Studio 2005
Version 8.0.50727.867  (vsvista.050727-8600)
Microsoft .NET Framework
Version 2.0.50727 SP2

Installed Edition: VC Express

Microsoft Visual C++ 2005   76542-000-0000011-00125
Microsoft Visual C++ 2005

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> Microsoft Visual Studio 2005
> Version 8.0.50727.867  (vsvista.050727-8600)

I'm not familiar with this: would it be the one included in Vista SDK?

I was referring to standalone Express, which 'cl' outputs:
*noSP: Version 14.00.50727.42
*SP1 : Version 14.00.50727.762

Yours (....867) seems newer!?

Comment 18

[Robert Longson [:longsonr]]

Mine is 14.00.50727.762

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 477529 [details] [diff] [review]
(Av1) Add an unrelated PRUint8() cast
[Checked in: Comment 26]

This bug blocks final.  This does not need explicit approval.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 477529 [details] [diff] [review]
(Av1) Add an unrelated PRUint8() cast
[Checked in: Comment 26]

(In reply to comment #7)
> I backed out all the changesets in your push

http://hg.mozilla.org/mozilla-central/rev/57b6fe67200c

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 477529 [details] [diff] [review]
(Av1) Add an unrelated PRUint8() cast
[Checked in: Comment 26]

http://hg.mozilla.org/mozilla-central/rev/6b23d13ab112

Comment 22

[Justin Lebar (not reading bugmail)]

Backed out -- not a b7/a2 blocker.

http://hg.mozilla.org/mozilla-central/rev/81a060f9cbaf

Comment 23

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> Backed out -- not a b7/a2 blocker.

Damn! How can that be when default branch is 2.0b8pre?

Comment 24

[Phil Ringnalda (:philor)]

http://groups.google.mn/group/mozilla.dev.planning/browse_thread/thread/3538e7b1f4fea779#

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #24)
> http://groups.google.mn/group/mozilla.dev.planning/browse_thread/thread/3538e7b1f4fea779#

"we decided to ship Firefox 4 Beta 7 from mozilla-central instead of the relbranch that we created on October 6th":
talk about confusion :-|

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 477529 [details] [diff] [review]
(Av1) Add an unrelated PRUint8() cast
[Checked in: Comment 26]

http://hg.mozilla.org/mozilla-central/rev/a591147ec48b

Comment 27

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> NB: I'm now wondering whether non-SP1 may be starting to have difficulties with
> our more recent code. I'll try to upgrade...

That was it: I filed bug 610936.

Comment 28

[Robert Longson [:longsonr]]

This has a patch so I'll just mark it fixed rather than invalid as we're not going to take any other patch that hacks up perfectly good code.