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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
SVG
--
minor
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

Trunk
mozilla2.0b7
x86
Windows 2000
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
(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;
(Assignee)

Updated

8 years ago
status2.0: --- → ?
(Assignee)

Comment 1

8 years ago
Iiuc, it looks like VC++ 2005 has difficulty w.r.t.
{
class DOMSVGLengthList : public nsIDOMSVGLengthList
{
  friend class DOMSVGLength;
}
Keywords: helpwanted
(Assignee)

Comment 2

8 years ago
Created attachment 477529 [details] [diff] [review]
(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.
Attachment #477529 - Flags: review?(longsonr)
Attachment #477529 - Flags: review?(longsonr) → review+
(Assignee)

Comment 3

8 years ago
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.
Attachment #477529 - Flags: approval2.0?
Assignee: nobody → sgautherie.bz
(Assignee)

Comment 4

8 years ago
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
Attachment #477529 - Attachment description: (Av1) Add an unrelated PRUint8() cast → (Av1) Add an unrelated PRUint8() cast [Checked in: Comment 4]
Attachment #477529 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Assignee: sgautherie.bz → nobody
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)
(Assignee)

Comment 6

8 years ago
(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...
I backed out all the changesets in your push because they weren't approved.
(Assignee)

Comment 8

8 years ago
(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' :-/
(Assignee)

Comment 9

8 years ago
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 :-<
Attachment #477529 - Flags: approval2.0?
(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)
(Assignee)

Comment 11

8 years ago
"blocking2.0=?":
Can't build on tier-1 platform.
blocking2.0: --- → ?
blocking2.0: ? → final+
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.
(Assignee)

Comment 13

8 years ago
(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".
I have Microsoft Platform SDK for Windows Server 2003 R2 installed.
(Assignee)

Comment 15

8 years ago
(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)
Depends on: 595708
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
(Assignee)

Updated

8 years ago
Depends on: 605569
(Assignee)

Comment 17

8 years ago
(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!?
Mine is 14.00.50727.762
Assignee: nobody → sgautherie.bz
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.
Attachment #477529 - Flags: approval2.0?
(Assignee)

Comment 20

8 years ago
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
(Assignee)

Comment 21

8 years ago
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
Attachment #477529 - Attachment description: (Av1) Add an unrelated PRUint8() cast [Checked in: Comment 4] → (Av1) Add an unrelated PRUint8() cast [Checked in: Comment 21]
Backed out -- not a b7/a2 blocker.

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

Comment 23

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

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

Comment 25

8 years ago
(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 :-|
Keywords: checkin-needed
Whiteboard: [c-n: after 2.0b7 freeze]
(Assignee)

Comment 26

8 years ago
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
Attachment #477529 - Attachment description: (Av1) Add an unrelated PRUint8() cast [Checked in: Comment 21] → (Av1) Add an unrelated PRUint8() cast [Checked in: Comment 26]
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [c-n: after 2.0b7 freeze]
(Assignee)

Updated

8 years ago
Assignee: sgautherie.bz → nobody
(Assignee)

Updated

8 years ago
Depends on: 610936
(Assignee)

Comment 27

8 years ago
(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.
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.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Summary: "DOMSVGLength.cpp(58) : error C2248: 'mozilla::DOMSVGLengthList::mItems' : cannot access private member declared in class 'mozilla::DOMSVGLengthList'" → Fix build warning - 'argument' : conversion from 'PRUint16' to 'PRUint8', possible loss of data
Severity: blocker → trivial
Keywords: helpwanted
(Assignee)

Updated

8 years ago
No longer depends on: 605569
(Assignee)

Updated

8 years ago
Assignee: nobody → sgautherie.bz
Severity: trivial → minor
status2.0: ? → ---
No longer depends on: 595708
Flags: in-testsuite-
Target Milestone: --- → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.