39424 - SUNWspro5.0 bustage with new string APIs

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[tor]

SUNWspro5.0 (native Sun compiler) is having problems with the new string APIs.
One of the problems is easily fixed:

Index: nsCharTraits.h
===================================================================
RCS file: /cvsroot/mozilla/xpcom/ds/nsCharTraits.h,v
retrieving revision 1.8
diff -u -r1.8 nsCharTraits.h
--- nsCharTraits.h      2000/05/13 20:17:35     1.8
+++ nsCharTraits.h      2000/05/16 17:28:51
@@ -307,7 +307,7 @@
     const char*
     find( const char* s, size_t n, char c )
       {
-        return NS_STATIC_CAST(char*, memchr(s, to_int_type(c), n));
+        return NS_STATIC_CAST(const char*, memchr(s, to_int_type(c), n));
       }
 
 #if 0

There is another one that it's tripping over in
netwerk/mime/src/nsXMLMIMEDataSource.cpp that seems a bit
more serious:

"../../../dist/include/nsAReadableString.h", line 1172: Error: Cannot cast from
basic_nsLiteralString<char> to const basic_nsAReadableString<char>&.
"../../../dist/include/nsString.h", line 800:     Where: While instantiating
"Compare<char>(const basic_nsAReadableString<char>&, const char*)".
"../../../dist/include/nsString.h", line 800:     Where: Instantiated from
non-template code.
1 Error(s) detected.

Comment 1

[Chris Waterson]

The first change looks like a no-brainer. As for the second, IIRC, we only 
needed this to resolve ambiguity on gcc-2.7.2.3 (but don't quote me on that: 
we'll need to run it through the gauntlet on Win32, egcs-1.1.2, and MetroWerks). 
We may be able to write an autoconf test that determines whether or not the 
ambiguity-resolving cast is required.

Comment 2

[Chris Waterson]

Let me clarify. We should be able to do something like:

#ifdef NEED_CPP_AMBIGUITY_RESOLVED_FOR_TEMPLATES /* XXX or something */
#define NS_READABLE_CAST(expr) NS_STATIC_CAST(/* that big-ass mess */)
#else
#define NS_READABLE_CAST(expr) expr
#endif

Comment 3

[Scott Collins]

Yeah, the first fix is a no-brainer.  I should have written it that way in the 
first place.  The fixed version is correct; the previous version has a |const| 
error.  I will check that in soon.  As for the second case, yes ... waterson and 
I have wrestled with this particular problem on different platforms for a while 
now.  We can do the right thing ... it just turns out to be different on every 
platform.

Comment 4

[tor]

*** Bug 39730 has been marked as a duplicate of this bug. ***

Comment 5

[Scott Collins]

The fix for the first case is checked in.  Now we just have to figure out what to 
do about the second case.

Comment 6

[Rich Burridge]

[richb - 5/24/00]
Scott/Chris,

If you could isolate the second case out into a small self-contained C++
program that reproduces/replicates the problem with the Sun compilers, I'll
file a P1/S1 bug against the compilers and get them to look at it right away.
Thanks.

Comment 7

[Rich Burridge]

[richb - 5/25/00]
I've filed a P1/S1 bug against the Sun compilers on this (4341384). It
also fails with Workshop 6 (ie. v5.1 of the Sun compilers).

I've received some comments from the Sun compiler guy who's been looking
at this:

"The code looks OK to me. I think it's a compiler bug, but I
can't reproduce it with a simple test case. The cast in the
failing expression isn't needed, and if you remove it, the
code compiles.

That is, the line in question expands to this:

return Compare ( lhs ,
    ( static_cast < const basic_nsAReadableString < CharT > & >
        ( ( basic_nsLiteralString < CharT > ( rhs ) ) ) ) ) ;

If you remove the static_cast, the code compiles:

return Compare ( lhs ,
        ( ( basic_nsLiteralString < CharT > ( rhs ) ) ) ) ;

I think the corresonding source code change would be changing this
    return Compare(lhs, NS_READABLE_CAST(CharT, 
	basic_nsLiteralString<CharT>(rhs)));

to this
    return Compare(lhs,  
	basic_nsLiteralString<CharT>(rhs));
"

Scott, is this an acceptable change, and would it work across all platforms?

Comment 8

[Rich Burridge]

[richb - 5/26/00]
I've changed the priority and severity of this bug to P1/critical.
This problem is preventing the Solaris build of Mozilla from compiling
when we use the Sun native compilers.

There is a proposed fix. Can this be reviewed/approved and  checked
in please?

Comment 9

[Chris Waterson]

Since scc is on vacation, I'll take a look at this.

Comment 10

[Scott Collins]

When we started, we weren't using a cast here at all.  It turned out to be 
required by some of the broken compilers to avoid `type-unification' errors.  We 
struggled to make the cast something palatable to all compilers.  It may be that 
we need to conditionally define it, e.g., to nothing, on platforms where it turns 
out to be problematic.

Shouldn't be too hard, _if_ we can write an autoconf test for it :-)

Comment 11

[Chris Waterson]

Attachment: proposed fix: add test for NEED_CPP_TEMPLATE_CAST_TO_BASE

Comment 12

[Chris Waterson]

richb, tor: could you try the attached patch and let me know if it does the job?
Works on gcc-2.7.2.3 and Linux.

Comment 13

[tor]

Tested with SUNWspro5.0; it does the right thing.

Comment 14

[Rich Burridge]

[richb - 5/26/00]
I second tor's findings. I downloaded last night's .tar.gz, and have just
finished building it with your patch with SC5.0 on a Solaris 7 (SPARC) machine.
It runs fine. Thankyou!

Comment 15

[Scott Collins]

I like it too!

Comment 16

[Chris Waterson]

fix checked in.

Comment 17

[Joerg.Brunsmann]

This patch breaks FreeBSD 3.4 with gcc version 2.7.2.3. See next attachment. If
I check out r1.55 of nsAReadableString.h it compiles.

Comment 18

[Joerg.Brunsmann]

Attachment: Build log

Comment 19

[Chris Waterson]

Sent mail to Joerg to verify that "configure" was re-run.

Comment 20

[Joerg.Brunsmann]

Ok, just re-run configure, pulled the latest nsAReadableString.h version, 
and it compiles just fine. Sorry for causing confusion.

Comment 21

[cls]

I'm still having problems getting a compile working with WS5.0 with the latest
tip.
Configure options: --enable-nspr-autoconf --enable-mathml --enable-svg
--with-extensions
Configure env args: CC=cc CXX=CC

CC -library=iostream  -mt  -DDEBUG -DDEBUG_cls -DTRACING -g -o regExport
regExport.o -xildoff -L../../../dist/bin -L../../../dist/lib  -L/space/ws50/lib
-L/usr/openwin/lib -R/usr/openwin/lib -lgtk -lgdk -lgmodule -lglib -ldl -lXext
-lX11 -lsocket -lnsl -lm -L../../../dist/bin -lplds4 -lplc4 -lnspr4 -lpthread
-L../../../dist/bin -lxpcom  -lw -lposix4 -lintl -lelf -lnsl -lsocket -lresolv
-liostream -lCrun -ldl -lm   
Undefined                       first referenced
 symbol                             in file
void basic_nsAWritableString<unsigned short>::do_AppendFromElement(unsigned
short) ../../../dist/bin/libxpcom.so
void basic_nsAWritableString<char>::do_AppendFromElement(char)
../../../dist/bin/libxpcom.so
ld: fatal: Symbol referencing errors. No output written to regExport

Comment 22

[Scott Collins]

Looks like WS5.0 has a little problem with template inlined virtual members.  Can 

you chop out the |inline| specification on |do_AppendFromElement| (at or around

<http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsAWritableString.h#563>).  Let 

me know if that fixes the problem, and I'll decide if I can live without it 

(probably) or if I need an auto-conf test.

Comment 23

[cls]

Oops.  My mistake.  After applying the OS patches listed at
http://www.mozilla.org/unix/solaris.html , the problem went away.