Closed Bug 65664 Opened 25 years ago Closed 24 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.
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.
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: 25 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: 25 years ago → 24 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.

Attachment

General

Creator:
Created:
Updated:
Size: