224968 - compiling nsFontMetricsXft.cpp with Sun Forte compilers fails due to inlined function definition *after* usage

Status: VERIFIED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Mitch]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.6a) Gecko/20031031
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.6a) Gecko/20031031

This has been happening for a while now, but i manually edit 
nsFontMetricsXft.cpp to overcome the problem. I thought it's about
time i should report it as a bug even though other compilers accept
the syntax, it is definetly incorrect.

We cannot inline a function if it is already used. Sun's Forte compilers barf
and treat this as a real fatal error - since we are not getting inline(ing)
as we requested and thus are left with a false sense of it actually working.

Although the functions it barfs on are empty, this should be fixed, or 
references to them removed from the relevant classes.

Refer to attachment for errors and incorrectly defined functions in lines
2109 and 2129 of mozilla/gfx/src/gtk/nsFontMetricsXft.cpp.

Three solutions exist.

1. Remove the inline keyword (my current fix for a clean compile)
2. Move the two functions to nearer the top of the file so they are
   defined as inline before being used by other classes in the file.
3. Remove the functions (and references to the callers) altogether since they
   appear to be redundant as they are empty definitions.

Please determine appropriate fix and checkin to cvs. Thank you.

Reproducible: Always

Steps to Reproduce:
1. Compile using Sun Forte CC compilers.
2.
3.

Actual Results:  
Build failure

Expected Results:  
Build success

Comment 1

[Mitch]

Attachment: Errors from a build using Sun Forte Compilers 5.0

Comment 2

[[e-mail]]

Attachment: patch

Comment 3

[[e-mail]]

Comment on attachment 134971 [details] [diff] [review]
patch

blizzard, care to r+sr this simple fix?

Comment 4

[[e-mail]]

-> me

Comment 5

[Mitch]

Any chance of getting this checked in ?

It appears to compile fine on SOS8 and Forte 5.x/6.x compilers.

Comment 6

[Mitch]

Try again. Any chance of having this fix integrated into the cvs tree please ?
The recent changes to nsFontMetricsXft.cpp means the included patch is failing.

Comment 7

[[e-mail]]

Comment on attachment 134971 [details] [diff] [review]
patch

trying a different reviewer.

alecf, care to r+sr?

Comment 8

[Alec Flett]

Comment on attachment 134971 [details] [diff] [review]
patch

pseudo-inline virtual declarations (i.e. putting the function body to a virtual
function inside a class declaration) are generally bad - it makes the compiler
insert a copy of the inline in every file that #include the declaration..

But I guess that's a whole other enchilada since this is a .cpp file. In any
case, inline virtuals are never actually inline, because you still need a
vtable entry at runtime and so the compiler will put it somewhere and still
make sure it goes through the vtable to ensure that the correct method is
called.

 My suggestion is to simply remove the 'inline' from the virtuals (As suggested
above) - moving them into the class declaration buys you nothing.

I suppose this patch doesn't HURT anything, you get r+sr=alecf, but I'm never a
big fan of non-trivial functions being declared inside the class declaration.

Comment 9

[[e-mail]]

Comment on attachment 134971 [details] [diff] [review]
patch

ok, thanks for the explanation... i'll keep the virtuals outside the class
declaration.

requesting approval for this simple compiler bustage fix.

Comment 10

[tor]

Comment on attachment 134971 [details] [diff] [review]
patch

a=tor for 1.6b checkin

Comment 11

[[e-mail]]

fixed.

Comment 12

[Mitch]

Thanks Dan for all the effort in comitting this back. Makes compiles
much easier than hand patching.