Closed
Bug 107291
Opened 23 years ago
Closed 22 years ago
|nsCOMPtr|: is it time to remove the 'don't forward declare T hack'?
Categories
(Core :: XPCOM, enhancement, P1)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: netscape, Assigned: john)
References
Details
(Whiteboard: [FIX])
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
sicking
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
sicking
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
I was trying to clean up circular jar requirement from bug 107289 and I ran across this tidbit in nsCOMPtr.h: enum { _force_even_compliant_compilers_to_fail_ = sizeof(T) }; /* The declaration above exists specifically to make |nsCOMPtr<T>| _not_ compile with only a forward declaration of |T|. This should prevent Windows and Mac engineers from breaking Solaris and other compilers that naturally have this behavior. Thank <law@netscape.com> for inventing this specific trick. Of course, if you're using |nsCOMPtr| outside the scope of wanting to compile on Solaris and old GCC, you probably want to remove the enum so you can exploit forward declarations. */ What exactly was the "tainting" problem and which compilers did it affect?
Comment 1•23 years ago
|
||
I believe the only compiler affected was gcc 2.7.2.3, which we dropped a few weeks ago. We could try removing this enum declaration and then putting an nsCOMPtr with a forward-declaration into the tree and seeing if any tinderboxes go red...
Reporter | ||
Updated•23 years ago
|
Priority: -- → P2
Target Milestone: --- → mozilla0.9.7
Reporter | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.7 → mozilla0.9.9
Reporter | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.9 → mozilla1.2
Reporter | ||
Comment 3•22 years ago
|
||
Nevermind.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → WONTFIX
Updated•22 years ago
|
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Comment 4•22 years ago
|
||
Why nevermind? We should fix this. I'll take it if you want (although I won't get to it immediately).
Comment 5•22 years ago
|
||
Taking.
Assignee: seawood → dbaron
Status: REOPENED → NEW
Priority: P2 → P1
Target Milestone: mozilla1.2alpha → Future
Comment 6•22 years ago
|
||
*** Bug 173154 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 7•22 years ago
|
||
This patch includes both nsCOMPtr<> and a short testcase for it in dlldeps.cpp (a fictional class)
Comment 8•22 years ago
|
||
Comment on attachment 102112 [details] [diff] [review] Patch dlldeps.cpp is not built cross-platform. Find a real test that's actually part of the build, and that you've tested fails without the nsCOMPtr.h changes.
Assignee | ||
Comment 9•22 years ago
|
||
This one changes a file that is compiled on all platforms and which I have tested does *not* work before and *does* work after the change.
Attachment #102112 -
Attachment is obsolete: true
Comment 10•22 years ago
|
||
Comment on attachment 102113 [details] [diff] [review] Patch #2 r/sr=dbaron
Attachment #102113 -
Flags: superreview+
Comment on attachment 102113 [details] [diff] [review] Patch #2 r=sicking assuming you've talked with scc about this and he's for it
Attachment #102113 -
Flags: review+
Assignee | ||
Comment 12•22 years ago
|
||
Fix checked in. Let's see what she does to ports, arrr.
Assignee: dbaron → jkeiser
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [FIX]
Assignee | ||
Comment 13•22 years ago
|
||
*sigh* OS/2 failed. Will back out once BeOS picks up the change so we can see what happens with that platform.. mkaply, what can we do about this? IBM stopped supporting the OS/2 compiler a year and a half ago ( http://www1.ibmlink.ibm.com/cgi-bin/master?xh=Fi7dVYO8vOv$7X1USenGnN9332&request=announcements&parms=H%5f901%2d013&xhi=usa%2emain&xfr=N ). Are there any other options? bbaetz also points out that this means we will need the hack in nsCOMArray.
Assignee | ||
Comment 14•22 years ago
|
||
Fix backed out. This is a serious issue. As we start to use more and more templates in our code, backwards platforms are going to hinder progress.
Comment 15•22 years ago
|
||
alecf: will we need this hack in nsCOMArray? I'm guessing yes, but I guess it depends on exactly what part of this construct os2 was breaking on.
Comment 16•22 years ago
|
||
Oh, and one other thing I mentioned to jkeiser - when we do enable this again, we should have an autoconf test which errors out if this won't compile, just to make the requirements explicit/easier to find/etc.
Comment 17•22 years ago
|
||
we might, actually. though in nsCOMArray's case, there is no member variable that depends on T, the only member variable is a nsVoidArray.
Comment 18•22 years ago
|
||
Can someone give me a simple non Mozilla testcase for this? Was this something that became allowed in later C++ standards? And yes our compiler does suck. But there is not much I can do about it. If what we are seeing is a genuine bug, I can still try to get it fixed. We are looking into other compilers, but are stuck with this one for now.
Assignee | ||
Comment 19•22 years ago
|
||
I *believe* this is a minimized testcase that will fail. The essential deal is, class A { TemplatizedClass<B> b; }; declaration should work until such time as the constructor / destructor actually have to be called. I guess the OS/2 compiler is compiling these things early. I'm a little surprised so many compilers do this, in fact :) mkaply, can you give this one a try? And yes, OS/2 is the only build that failed to compile ...
Comment 20•22 years ago
|
||
Here is the statement from our compiler folks: According to the development this works as designed. The development have returned this defect claim with explanation that IBM C++ compilers 3.6 used a template instantiation method where all member functions of the class were always instantiated. Unlike the newer, ISO-14882 compliant compilers, the 3.6 compiler doesn't use the rule of only instantiating the referenced member functions. So, if we go this way, the OS/2 port of Mozilla will break until we can switch to a new compiler - we have no timeframe for this.
Comment 21•22 years ago
|
||
Well, since the ibm compiler appears to have been EOL'd for OS2, would it be reasonable to assume that any solution would involve using an entirely different compiler? (timeless mentioned openwatcom)
Comment 22•22 years ago
|
||
Yes, right now we are looking at OpenWatcom. We have no timeframe though.
Comment 23•22 years ago
|
||
I got timeless to test, and his version of openwatcom (Watcom C16 Optimizing Compiler Version 11.0c) doesn't handle this test case. It also doesn't seem to know about the true/false boolean types. Its possible that anextra option of some sort was needed, though - it was a really quick test. OTOH, a quick browse through the docs at http://www.openwatcom.org/support/reference_content.html shows functions called |isEmpty| with an int return type, which isn't very hopeful. gcc doesn't work on os2, either, although there are patches for pgcc for 2.95.3. pgcc was gcc + 'pentium specific optimisations'. That was released in Jan 2000, and seems to be unmaintained. theres also a 2.8.1 binary downloadable. Sigh.
Assignee | ||
Comment 24•22 years ago
|
||
gcc 2.95 compiles Mozilla, what's wrong with that? Unless the patch sucks, maybe that will work better than VisualAge :) timeless, you have a copy of that around?
Comment 25•22 years ago
|
||
We're looking at bring the OS/2 GCC build back. It's just a matter of time.
Assignee | ||
Comment 26•22 years ago
|
||
That would be great :)
Comment 27•22 years ago
|
||
collecting under the |nsCOMPtr| tracking bug # 178174
Blocks: nsCOMPtr_tracking
Summary: cannot forward declare nsCOMPtr classes → |nsCOMPtr|: is it time to remove the 'don't forward declare T hack'?
Updated•22 years ago
|
Severity: normal → enhancement
Assignee | ||
Comment 28•22 years ago
|
||
Setting dependent on bug 107291, "drop VisualAge on OS/2"
Depends on: 179118
Comment 29•22 years ago
|
||
This hack is also present in nsAutoPtr.h
Assignee | ||
Comment 30•22 years ago
|
||
This patch does the same thing for nsRefPtr and friends. sicking has given r= already. There are no other copies of the hack that I can find in lxr.
Comment 31•22 years ago
|
||
Does the AIX compiler have the same bug as the VisualAge compiler? (The AIX tinderbox recently disappeared from SeaMonkey-Ports, so be careful...)
Comment 32•22 years ago
|
||
We were having networking problems today which is why the Tinderbox went down. I am restarting it now. I will try the testcase in this defect against the AIX VisualAge C++ v6 compiler.
Assignee | ||
Comment 33•22 years ago
|
||
We'll have to find out ... I checked in the nsCOMPtr patch last night, so when the tinderbox cycles we shall see.
Assignee | ||
Comment 34•22 years ago
|
||
Comment on attachment 124785 [details] [diff] [review] nsAutoPtr.h Patch I will not check this in until we verify that AIX has cleared, but the VA people told us before that they had fixed this bug in later versions of the compiler, so v6 is probably OK. Nonetheless, I'd like to get r=/sr= now so I *can* check this in when that is verified.
Attachment #124785 -
Flags: superreview?(dbaron)
Attachment #124785 -
Flags: review?(bugmail)
Updated•22 years ago
|
Attachment #124785 -
Flags: superreview?(dbaron) → superreview+
Comment on attachment 124785 [details] [diff] [review] nsAutoPtr.h Patch you should probably forwarddeclare some user of nsAutoPtr too. There's a bunch in transformiix which is XP
Attachment #124785 -
Flags: review?(bugmail) → review+
Comment 36•22 years ago
|
||
Yesterday's xpcom patch built fine on AIX - we didn't have any errors while building today's daily build. I have appled the patch to nsAutoPtr.h to a build tree and rebuilt several files which use nsAutoPtr.h (including extensions/transformiix/source/xslt/*.cpp). I have not rebuilt the entire tree with the nsAutoPtr.h patch, but everything seems to work fine.
Assignee | ||
Comment 37•22 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 38•22 years ago
|
||
OK, so there is a problem which we're trying to figure out if we can get around: on gcc2.9x and below, a class with an automatic destructor and a forward-declared nsCOMPtr will fail to compile because the destructor is compiled when declared, not when used. This showed up in about 15% of the XSLT changes (bug 208188).
Comment 39•22 years ago
|
||
there's a bug about ABI warnings which relates to these automatic destructors, perhaps the simplest fix for all of the problems is to simply write the empty automatic destructors.
Assignee | ||
Comment 40•21 years ago
|
||
That would solve it (I presume you mean declaring all destructors in these classes and then defining them in the .cpp), but if you *don't* follow that rule you will break gcc2.9x Linux and waste your own time. So ... is a C++ portability rule enough? We have other things you can do that break platforms, but this one *is* preventable. sicking has a great idea to make COMPtrs not depend on the type when destructing (reinterpret_cast to nsISupports before calling AddRef / Release). That would allow us to keep the hack out of nsCOMPtr. Unfortunately for nsAutoPtr and nsRefPtr there is no such recourse; we'd have to rely on a style rule or a sizeof hack there. Another thing that could be done is focus the hack so that it only happens when the RefPtr / AutoPtr is *destructed* and there is no type information ... but that probably wouldn't work because as we have seen the automatic destructor is not compiled until and unless it is needed on the other platforms. I'll give it a whirl anyway, on the off chance.
Comment 41•21 years ago
|
||
The ABI warnings aare because (basically), if you don't declare a destructor, the compiler has to place a default one somewhere in the vtable. The spec says to put it at the end; instead it puts it at the beginning. At some point gc will move to follow the spec, and that will change the ABI. See teh ABI bug for more.
You need to log in
before you can comment on or make changes to this bug.
Description
•