Closed Bug 65664 Opened 23 years ago Closed 23 years ago

nsCOMPtr: make operator== always work with gcc

Categories

(Core :: XPCOM, defect, P2)

Product:
Core
Component:
XPCOM
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.8

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(10 files)

testcase that causes error when compiled with `g++ -pedantic`
388 bytes, text/plain
Details
autoconf test for the bug
2.02 KB, patch
Details | Diff | Splinter Review
patch that seems to help (but will it break const cases?)
1.28 KB, patch
Details | Diff | Splinter Review
TestCOMPtrEq.cpp
3.88 KB, text/plain
Details
possible fix (need to look at const issues on, e.g., HPUX)
3.81 KB, patch
Details | Diff | Splinter Review
same patch with better comments
4.61 KB, patch
Details | Diff | Splinter Review
testcase demonstrating bug, for compiler bug reports (is this correct?)
1.31 KB, text/plain
Details
revised patch - uses simpler test and better name for result
4.62 KB, patch
Details | Diff | Splinter Review
patch to re-enable the changes
1.08 KB, patch
Details | Diff | Splinter Review
proposed correct fix
4.78 KB, patch
Details | Diff | Splinter Review 
The code in nsCOMPtr.h that leads to certain errors about ambiguous operator==,
for example:

ambiguous overload for `nsCOMPtr<nsIAtom> & == nsIAtom *&'
candidates are: 
 operator==(nsIAtom *, nsIAtom *) <builtin>
../../../dist/include/nsCOMPtr.h:1164:
 NSCAP_BOOL operator== (const nsCOMPtr<T> &, const U *) [with T = nsIAtom, U =
 nsIAtom]

or:

ambiguous overload for `nsCOMPtr<nsIContent> & == nsDerivedSafe<nsIContent> *'
candidates are:
operator ==(nsDerivedSafe<nsIContent> *, nsDerivedSafe<nsIContent> *) <builtin>
../../../../dist/include/nsCOMPtr.h:1152:
 operator ==<nsIContent, nsDerivedSafe<nsIContent>>(const nsCOMPtr<nsIContent> &,
 const nsDerivedSafe<nsIContent> *)

The code in nsCOMPtr.h that's causing this breakage is |#ifndef
NSCAP_NSCOMPTR_TO_RAW_COMPARISONS_ARE_AMBIGUOUS|.  It would be nice to write
an autoconf test to set this so we don't have to deal with this breakage.
I'd like to investigate whether this is possible...

scc:  have you investigated this before?
My search for a testcase for this bug was greatly delayed by the fact that it
requires the -pedantic option to g++.  Without -pedantic, this compiles fine.

    
    

    
  
scc:  BTW, are you *sure* this is a bug?

    
    

    
  
Well, now I get the error:

/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHTTPChannel.cpp: In method
`nsresult 
nsHTTPChannel::Redirect (const char *, nsIChannel **, int)':
/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHTTPChannel.cpp:1775: no
match for 
`nsCOMPtr<nsIChannel> & == nsHTTPChannel *const'
/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHTTPChannel.cpp:1775:
candidates are: 
operator==(nsHTTPChannel *, nsHTTPChannel *) <builtin>
/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHTTPChannel.cpp:1775:    
            
operator==(nsDerivedSafe<nsIChannel> *, nsDerivedSafe<nsIChannel> *) <builtin>
../../../../dist/include/nsCOMPtr.h:1204:                 NSCAP_BOOL operator==
(const nsCOMPtr<T> &, 
NSCAP_Zero *) [with T = nsIChannel]
../../../../dist/include/nsXPIDLString.h:205:                 PRBool operator==
(const PRUnichar *, 
const nsXPIDLString &)
../../../../dist/include/nsXPIDLString.h:212:                 PRBool operator==
(const nsXPIDLString &, 
const PRUnichar *)
../../../../dist/include/nsXPIDLString.h:353:                 PRBool operator==
(const char *, const 
nsXPIDLCString &)
../../../../dist/include/nsXPIDLString.h:360:                 PRBool operator==
(const nsXPIDLCString 
&, const char *)
gmake: *** [nsHTTPChannel.o] Error 1


    
    

    
  
Well, at least I see why it's hard now.  Maybe a comment in nsCOMPtr.h near that
|#ifndef| would be nice...

    
    

    
  
Accepting so I try to remember to file gcc bugs on the issues that block us
from doing this...
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9

    
    

    
  
The HP native compiler seems to have the same problem, and it gives the error:

Error 229:
"/export1/tinderbox/SeaMonkey/HP-UX_B.10.20_Clobber/mozilla/xpfe/appshell/src/nsXULWindow.cpp",
line 822 # Ambiguous overloaded function call; a function match was not found
that was strictly best for ALL arguments. Two functions that matched best for
some arguments (but not all) were "int operator
!=<nsIXULWindow,nsDerivedSafe<nsIXULWindow> >(const nsDerivedSafe<nsIXULWindow>
*,const nsCOMPtr<nsIXULWindow> &)" ["../../../dist/include/nsCOMPtr.h", line
1175] and "bool operator !=(nsDerivedSafe<nsIXULWindow>
*,nsDerivedSafe<nsIXULWindow> *)" [Built-in operator].
          if (listXULWindow.get() != ourXULWindow) {
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^   


    
    

    
  
I got the idea to try this from the HP error message above and from section
13.3.3.2 of the C++ spec, clause 3,  (it's the online draft I'm looking at),
which says:

Standard conversion sequence S1 is a better conversion sequence than standard
conversion sequence S2 if
 ...
 * S1 and S2 differ only in their qualification conversion and yield similar
   types T1 and T2 (4.4), respectively, and the cv-qualification signature
   of type T1 is a proper subset of the cv-qualification signature of type
   T2.

I originally tried having different operator= with different const
qualifications, but that caused a comparison of a const to a non-const
to use the non-const version (which gave an error in the NS_STATIC_CAST
that I changed to cast to |void*| rather than |const void *|).  I
would have expected it to choose the other (const) one.  In other words,
I still really don't understand this...

    
    

    
  
Well, my entire tree built with that patch, and it passed a few additional tests
that usually cause nsCOMPtr bustage.

However, scc pointed out on IRC yesterday that perhaps the better solution would
be to break comparison of nsCOMPtrs of different types, since that's bad
practice anyway.

    
    

    
  

      
      
      
      

        Attached file
          
        TestCOMPtrEq.cpp
        — 
      

    


  

    
    

    
  
I've tried to come up with a solution where we don't support comparison across
types but we do support comparison across types with nsCOMPtr<nsISupports>, and
I don't see one, at least not immediately.  Introducing additional operator==
for the nsCOMPtr<nsISupports> cases breaks other things.  We could go with a
solution where comparison across types only breaks on Linux/HPUX with certain
combinations, but that wouldn't be great either.

    
    

    
  
For reference, the HPUX 10.20 tinderbox went red.  The HPUX 11.00 tinderbox
gave the message:

Error (future) 229: "nsXULWindow.cpp", line 822 # "Ambiguous overloaded function
call; a function match was not found that was strictly best for ALL arguments.
Two functions that matched best for some arguments (but not all) were "int
operator !=<nsIXULWindow,nsDerivedSafe<nsIXULWindow> >(const
nsDerivedSafe<nsIXULWindow> *,const nsCOMPtr<nsIXULWindow> &)"
["../../../dist/include/nsCOMPtr.h", line 1175] and "bool operator
!=(nsDerivedSafe<nsIXULWindow> *,nsDerivedSafe<nsIXULWindow> *)" [Built-in
operator]." Choosing "int operator !=<nsIXULWindow,nsDerivedSafe<nsIXULWindow>
>(const nsDerivedSafe<nsIXULWindow> *,const nsCOMPtr<nsIXULWindow> &)"
["../../../dist/include/nsCOMPtr.h", line 1175] for resolving ambiguity.
          if (listXULWindow.get() != ourXULWindow) {
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^   
...
Warning:        1 future errors were detected and ignored. Add a '+p' option to
detect and fix them before they become fatal errors in a future release.
Behavior of this ill-formed program is not guaranteed to match that of a
well-formed program

It would sure be nice to get the warnings list from this tinderbox posted on the
tinderbox and blamed :-).  (We'd probably have to write a parser for its error
format.)

    
    

    
  
OK, I agree that this is a bug in gcc/aCC.  Here are the quotes from
the C++ spec that convinced me:

13.6, clause 16:

  For every pointer type T, there exist candidate operator functions
  of the form

    ...
    bool operator==(T, T);
    bool operator!=(T, T);


13.1, clause 3, bullet 3:
  [[In this section, equivalent refers to function declarations that
  cannot be overloaded because they are equivalent.  It's also a
  Note, referring to 8.3.5, clause 3]]

  Parameter declarations that differ only in the presence or absence
  of |const| and/or |volatile| are equivalent.    That is, the |const|
  and |volatile| type-specifiers for each parameter are ignored when
  determining which function is being declared, defined, or called.

  Only the |const| and |volatile| type-specifiers at the outermost level
  of the parameter type specification are ignored in this fashion;
  |const| and |volatile| type-specifiers buried within a parameter type
  specification are significant


8.3.5, clause 3:

  After producing the list of parameter types, several transformations
  take place upon these types to determine the function type.
  Any cv-qualifier modifying a parameter type is deleted; e.g., the
  type |void(const int)| becomes |void(int)|.  Such cv-qualifiers only
  affect the definition of the parameter within the body of the function;
  they do not affect the function type.


13.3.3.1, clause 6:

  Any difference in top-level cv-qualification is subsumed by the
  initialization itself and does not constitute a conversion.  [Example:
  a parameter of type |A| can be initialized from an argument of type
  |const A|.  The implicit conversion sequence for that case is the
  identity sequence; it contains no "conversion" from |const A| to |A|.]


    
  
Target Milestone: mozilla0.9 → mozilla0.8

    
    

    
  
r=scc

    
    

    
  
This bug seems to be fixed already on the gcc trunk.

    
    

    
  
I take that back.

    
    

    
  
The current status of testing of this patch is:
 * it works on gcc 2.96-RH and gcc trunk
 * gcc 2.7.2.3 passes the autoconf test, so it has no effect there, and gcc
2.7.2.3 builds TestCOMPtrEq correctly (at least in my possibly-messed-up partial
install of gcc 2.7.2.3)

I'll try to test it on gcc 2.91.66 (egcs 1.12) tonight.  If someone wants to
test it on HP's aCC (which I think it would affect) or other compilers (I don't
know of others where it would cause changes), go ahead, but I'm prepared to risk
just checking it in early on a Sunday morning and hoping things go well. (After
all, what's a little Sunday morning bustage in the name of preventing lots of
other bustage?)  Since all the changes are based on an autoconf test, at worst,
I'll have to modify the autoconf test or pull TestCOMPtrEq back out of the
default build.

But cc:ing jdunn and cls in case they disagree with that proposal and want to
help test on other compilers to which I don't have access.

    
    

    
  
I am testing the aCC +p option right now...

    
    

    
  
sr=brendan@mozilla.org on last patch.  Am I right to infer that cv-qualifier
stands for const or volatile qualifier?  If so, should we have a define (perhaps
not in nsCOMPtr.h) for NSCAP_VOLATILE_PARAM too?

/be

    
    

    
  
Yes, cv-qualifier does stand for const or volatile.  NSCAP_VOLATILE_PARAM might
be useful if we wanted to make arguments volatile within the scope of functions
that were causing ambiguity, but we don't right now, so I'd rather not add it
until it's needed (since I don't expect it to be needed in the near future).

    
    

    
  
Urk.  The HP build that builds tests is the one without +p, and it gives the
warnings:

Error (future) 581: "TestCOMPtrEq.cpp", line 84 # Ambiguous overloaded function
call; under the Standard rules, two functions each have a parameter which is a
better match for the corresponding argument. However, for the function "bool
operator ==(nsICOMPtrEqTestFoo *,nsICOMPtrEqTestFoo *)" [Built-in operator],
this difference is only due to not needing an lvalue-to-rvalue conversion, which
is a relatively new rule. The function "int operator
==<nsICOMPtrEqTestFoo,nsICOMPtrEqTestFoo>(const nsCOMPtr<nsICOMPtrEqTestFoo>
&,const nsICOMPtrEqTestFoo *)" ["../../dist/include/nsCOMPtr.h", line 1176] is
better for other reasons, and so has been selected to preserve compatibility
during the transition to the new rule.
                  (s == r) &&
                   ^^^^^^    

I don't yet understand this error message, but it seems wrong to me at first
glance.  However, I'm still considering backing out the autoconf test so we
don't break +p on HP once people start checking in comparisons that would have
needed .get().

    
    

    
  
Since this build doesn't use +p, the autoconf test doesn't put in the |const|. 
So perhaps if the build used +p, the removal of the |const| would fix the
problem.  But I don't see why the error message is different from the error
I pasted above from the same machine.

    
    

    
  
I disabled the changes since it's not clear whether they might cause some HPUX
build configurations to break.  The above patch will re-enable them.

    
    

    
  
I have retested with the latest patch on 11.00 & 10.20.  Both
compile fine.  The interesting thing is that I can compile on
both platforms with +p WITHOUT the fixes and they build so
enough changes have occured that on HP we don't even need this fix.

So as soon as this is checked in, I will change the tinderboxes to
start compiling with +p.



    
    

    
  
I suspect you were compiling with --disable-tests.  Could you try enabling the
tests or just building TestCOMPtrEq in xpcom/tests ?  That should fail with +p
without the patch.

    
    

    
  
Never mind.  The patch enables the test too, so it wouldn't break without the
patch because it's not enabled.  :-)

    
    

    
  
Correct, I wasn't building the tests.  So I enabled them.

If I compile TestCOMPtrEq.cpp this is what I get...
NOTE: +p and CPP_CV_QUALIFIERS_CAUSE_AMBIGUITY=1

TestCOMPtrEq.cpp
aCC -ext +p +DA1.1 +DS2.0 -o TestCOMPtrEq.o -c -DOSTYPE=\"HP-UXB.11\" -DOJI 
-DUSE_NSREG   -I../../dist/include -I../../dist/include      
-I/builds/jdunn/trunk/mozilla/xpcom/tests/../public 
-I/builds/jdunn/trunk/mozilla/xpcom/tests/services    +Z  -DHPUX11 -Dhpux  
-DNDEBUG -DTRIMMED -DMOZILLA_CLIENT -DNSCAP_DISABLE_DEBUG_PTR_TYPES=1 
-DD_INO=d_ino -DMOZ_WIDGET_GTK=1 -DMOZ_ENABLE_XREMOTE=1 
-DMOZ_DEFAULT_TOOLKIT=\"gtk\" -DSTDC_HEADERS=1 -DHAVE_ST_BLKSIZE=1 
-DHAVE_INT16_T=1 -DHAVE_INT32_T=1 -DHAVE_INT64_T=1 -DHAVE_UINT=1 -DHAVE_UINT_T=1 
-DHAVE_UINT16_T=1 -DHAVE_DIRENT_H=1 -DHAVE_SYS_BYTEORDER_H=1 -DHAVE_MEMORY_H=1 
-DHAVE_UNISTD_H=1 -DHAVE_SYS_STATVFS_H=1 -DHAVE_SYS_VFS_H=1 -DHAVE_SYS_MOUNT_H=1 
-DHAVE_LIBM=1 -DMOZ_OJI_REQUIRE_THREAD_SAFE_ON_STARTUP=1 -DHAVE_RANDOM=1 
-DHAVE_QSORT=1 -DHAVE_STRERROR=1 -DHAVE_LCHOWN=1 -DHAVE_FCHMOD=1 
-DHAVE_SNPRINTF=1 -DHAVE_LOCALTIME_R=1 -DHAVE_STATVFS=1 -DHAVE_MEMMOVE=1 
-DHAVE_USLEEP=1 -DHAVE_RINT=1 -DHAVE_GETTIMEOFDAY=1 -DGETTIMEOFDAY_TWO_ARGS=1 
-DHAVE_VALLOC=1 -DHAVE_IOS_BINARY=1 -DHAVE_OSTREAM=1 -DHAVE_CPP_EXPLICIT=1 
-DHAVE_CPP_SPECIALIZATION=1 -DHAVE_CPP_MODERN_SPECIALIZE_TEMPLATE_SYNTAX=1 
-DHAVE_CPP_PARTIAL_SPECIALIZATION=1 -DHAVE_CPP_ACCESS_CHANGING_USING=1 
-DHAVE_CPP_AMBIGUITY_RESOLVING_USING=1 -DHAVE_CPP_UNAMBIGUOUS_STD_NOTEQUAL=1 
-DHAVE_CPP_NEW_CASTS=1 -DHAVE_CPP_DYNAMIC_CAST_TO_VOID_PTR=1 
-DNEED_CPP_UNUSED_IMPLEMENTATIONS=1 -DHAVE_I18N_LC_MESSAGES=1 -DMOZ_MAIL_NEWS=1 
-DMOZ_ENDER_LITE=1 -DNS_MT_SUPPORTED=1 -DCPP_CV_QUALIFIERS_CAUSE_AMBIGUITY=1 
-DMOZ_USER_DIR=\".mozilla\" -DMOZ_MATHML=1 -DMOZ_SVG=1 -DMOZ_DLL_SUFFIX=\".sl\" 
-DXP_UNIX=1 -DUNIX_ASYNC_DNS=1 -DHAVE_MOVEMAIL=1 -DJS_THREADSAFE=1  
/builds/jdunn/trunk/mozilla/xpcom/tests/TestCOMPtrEq.cpp
Error 201: "/builds/jdunn/trunk/mozilla/xpcom/tests/TestCOMPtrEq.cpp", line 101 
# Pointer operands must be of the same type for operator '=='; comparison of 
'nsICOMPtrEqTestFoo *' and 'nsICOMPtrEqTestFoo2 *'.
                  (r == r2) &&
                   ^^^^^^^    
gmake: *** [TestCOMPtrEq.o] Error 2



    
    

    
  
How did it hit that error?  That code is commented out (see the #undef near the
top), and it's also on line 141 rather than line 101.

    
    

    
  
Maybe it is commented out in your version of TestCOMPtrEq.cpp but 
not in the one posted in this bug on 1/31/01 06:46

    
    

    
  
Could you test with the one that's checked in to the tree?  All you need to test
is the last patch on this bug.

    
    

    
  
Ok that works...
If I compile with +p and the CPP_CV_QUALIFIERS_CAUSE_AMBIGUITY=1
it compiles fine (and runs).
If I remove the CPP_CV_QUALIFIERS_CAUSE_AMBIGUITY=1 define, then I
have to also remove the +p.

This looks good to me (sorry for my confusion).

    
    

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

    
    

    
  
Reenabled the fix yesterday.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
It turns out my understanding of the standard was wrong -- so my previous fix
was actually testing for correct behavior rather than a bug.  (I misunderstood
the meaning of top-level cv-qualifiers.  It makes more sense if in "cv1 T * cv2"
the cv2 is the top-level cv-qualifier, but I was thinking it was the cv1.)

So I think I have a better fix that conforms to the standard, and I think we
even already have the autoconf test that will fix the fact that it would
otherwise fail on IRIX...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  


  

    
    

    
  
sr=scc; this will be a good thing

    
    

    
  
Jamie Was Right. r=waterson

    
    

    
  
Well, it turned out this caused problems on gcc 2.7.2.x and VC++, but I added an
autoconf test that should fix the problem (and an #ifdef _MSC_VER).

    
    

    
  
Fix checked in 2001-04-07 08:29 and 10:25 PDT.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Blocks: 803695
Depends on: 1013065

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