Patch to netwerk/base/public/nsNetUtil.h needed to be able to compile with Sun Forte 6 Update 2

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: riot.nrrrd.bugzilla, Assigned: bz)

Tracking

({fixed1.8.0.4, fixed1.8.1})

Trunk
Sun
Solaris
fixed1.8.0.4, fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O) Gecko/20040906 Camino/0.8+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O) Gecko/20040906 Camino/0.8+

While trying to compile Firefox 1.5.0.2 on Solaris 9 with Sun's older Forte 6 Update 2 compiler suite (we cannot upgrade, due to configuration freezes), I hit this problem:

nsInputStreamChannel.cpp
CC -o nsInputStreamChannel.o -c  -DMOZILLA_INTERNAL_API
-DOSTYPE=\"SunOS5\" [...]
nsInputStreamChannel.cpp

"nsInputStreamChannel.cpp", line 395: Error: Could not find a match for
NS_QueryNotificationCallbacks(nsCOMPtr<nsIInterfaceRequestor>,
nsCOMPtr<nsILoadGroup>, nsCOMPtr<nsIProgressEventSink>).

1 Error(s) detected.
gmake[5]: *** [nsInputStreamChannel.o] Error 1
gmake[5]: Leaving directory
`/usr/local/src/WWW/mozilla/1.8.0-Firefox/mozilla/netwerk/base/src'

There is another instance (if this instance is fixed) where another file does not compile with Forte 6 Update 2.  A second patch to nsNetUtil.h is required to fix that instance.

Reproducible: Always

Steps to Reproduce:
1. Try to build Firefox 1.5.0.2 from scratch using Sun Forte 6 Update 2.


Actual Results:  
See Details for compilation error.

Expected Results:  
Expected the code to compile cleanly  :-)


I propose this patch, to netwerk/base/public/nsNetUtil.h.  It fixes the problem with both files being unable to be compiled with Sun Forte 6.  Hopefully it will not break other compilers such as GCC on Suns or other OS platforms, or newer versions of Sun's Forte & Workshop compiler suites.

*** netwerk/base/public/nsNetUtil.h.dist        Mon Jul 25 13:27:02 2005
--- netwerk/base/public/nsNetUtil.h     Thu Apr 20 03:14:58 2006
***************
*** 979 ****
--- 980,996 ----
+ /* template helper */
+ template <class T> inline void
+ NS_QueryNotificationCallbacks(nsCOMPtr<nsIInterfaceRequestor> aCallbacks,
+                               nsCOMPtr<nsILoadGroup>          aLoadGroup,
+                               nsCOMPtr<T>           &aResult)
+ {
+     NS_QueryNotificationCallbacks(aCallbacks.get(), aLoadGroup.get(), aResult);
+ }
+ 
+ /* template helper */
+ template <class T> inline void
+ NS_QueryNotificationCallbacks(nsCOMPtr<nsIChannel> aChannel,
+                               nsCOMPtr<T>           &aResult)
+ {
+     NS_QueryNotificationCallbacks(aChannel.get(), aResult);
+ }
+
(Reporter)

Comment 1

11 years ago
Created attachment 219342 [details] [diff] [review]
Proposed patch to netwerk/base/public/nsNetUtil.h

The proposed patch allows Firefox 1.5.0.2 to compile cleanly with Sun Forte 6 Update 2.  Its impact on building with other compilers such as GCC or newer versions of Sun's Forte/Workshop compiler suite is presently unknown.
passing non-reference comptrs around is not such a great idea, it causes an additional addref and release...

also, please make patches with -pu6 as diff flags or something

Also... are both versions (with raw pointer and nsCOMPtr) needed, or do all callers have a comptr anyway?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Other → Solaris
Hardware: Other → Sun
(Reporter)

Comment 3

11 years ago
Comment on attachment 219342 [details] [diff] [review]
Proposed patch to netwerk/base/public/nsNetUtil.h

--- mozilla/netwerk/base/public/nsNetUtil.h.dist        Mon Jul 25 13:27:02 2005
+++ mozilla/netwerk/base/public/nsNetUtil.h     Thu Apr 20 03:14:58 2006
@@ -974,12 +974,29 @@ NS_QueryNotificationCallbacks(nsIInterfa
 {
     NS_QueryNotificationCallbacks(aCallbacks, aLoadGroup,
                                   NS_GET_IID(T),
                                   getter_AddRefs(aResult));
 }
 
+/* template helper */
+template <class T> inline void
+NS_QueryNotificationCallbacks(nsCOMPtr<nsIInterfaceRequestor> aCallbacks,
+                              nsCOMPtr<nsILoadGroup>          aLoadGroup,
+                              nsCOMPtr<T>           &aResult)
+{
+    NS_QueryNotificationCallbacks(aCallbacks.get(), aLoadGroup.get(), aResult);
+}
+
+/* template helper */
+template <class T> inline void
+NS_QueryNotificationCallbacks(nsCOMPtr<nsIChannel> aChannel,
+                              nsCOMPtr<T>           &aResult)
+{
+    NS_QueryNotificationCallbacks(aChannel.get(), aResult);
+}
+
 /**
  * This function returns a nsIInterfaceRequestor instance that returns the
  * same result as NS_QueryNotificationCallbacks when queried.  It is useful
  * as the value for nsISocketTransport::securityCallbacks.
  */
 inline nsresult
(Reporter)

Comment 4

11 years ago
(In reply to comment #2)
> passing non-reference comptrs around is not such a great idea, it causes an
> additional addref and release...
> 
> also, please make patches with -pu6 as diff flags or something

Sorry about that ... I updated the attachment (or tried to anyway - my first-ever Mozilla bug report, so I apologize for any blunders).

> Also... are both versions (with raw pointer and nsCOMPtr) needed, or do all
> callers have a comptr anyway?

Christian,

I have no clue ...I don't profess to understand either the code in nsInputStreamChannel.cpp nor the patch, just that it appears that Forte 6 needs some sort of Template helper for the 2 particular cases where it stumbles.  A code guru can undoubtedly make better sense of it than I - I am just passing on the patch I got from Boris Zbarsky of MIT <bzbarsky@mit.edu> over in the mozilla.dev.build newsgroup.

You can't change attachments once attached. Bugzilla's user interface is a bit confusing there, but your only option is to attach a new version.

cc'ing bz as this seems to be his patch :) see comment 2
I'll buy wanting references; let's do that.  I have no clue on the other; I'd have to audit the code.  It doesn't seem worth trying to do that to me, since all this stuff is inlined anyway....

reporter, want to attach an updated patch that uses "nsCOMPtr<whatever>&" instead of "nsCOMPtr<whatever>"?  At least assuming that still compiles for you?
(Reporter)

Comment 7

11 years ago
Created attachment 219363 [details] [diff] [review]
New "-pu6" version of proposed patch

"diff -pu6" version attached.

Can you and Boris discuss what the actual patch contents should be, Christian? :)

(I am willing to take an altered patch and do a complete rebuild to test it)
(Reporter)

Comment 8

11 years ago
(In reply to comment #6)
> reporter, want to attach an updated patch that uses "nsCOMPtr<whatever>&"
> instead of "nsCOMPtr<whatever>"?  At least assuming that still compiles for
> you?

Hi Boris,

If you guys want to submit a modified version, I'm willing to test it out and do a complete rebuild to verify.  I just want it to work; you guys can make it "correct" :)

Greg


Created attachment 219395 [details] [diff] [review]
So does this make Sun Forte work?
Attachment #219342 - Attachment is obsolete: true
Attachment #219363 - Attachment is obsolete: true
(Reporter)

Comment 10

11 years ago
(In reply to comment #9)
> Created an attachment (id=219395) [edit]
> So does this make Sun Forte work?

Yes, it works - I just finished a complete rebuild from scratch with no compilation problems.

(Now if only it didn't core dump when I click on the "Security" Preference ... but that's a different issue :-) )

Thanks everyone ...

Attachment #219395 - Flags: superreview?(darin)
Attachment #219395 - Flags: review?(darin)

Comment 11

11 years ago
Comment on attachment 219395 [details] [diff] [review]
So does this make Sun Forte work?

Should the "in" nsCOMPtr's be |const| ?
Created attachment 219518 [details] [diff] [review]
Probably a good idea.

Greg, would you mind testing this too?
Attachment #219395 - Attachment is obsolete: true
Attachment #219518 - Flags: superreview?(darin)
Attachment #219518 - Flags: review?(darin)
Attachment #219395 - Flags: superreview?(darin)
Attachment #219395 - Flags: review?(darin)

Comment 13

11 years ago
Comment on attachment 219518 [details] [diff] [review]
Probably a good idea.

r+sr=darin provided it works ;-)  ... please fix indenting of aResult
Attachment #219518 - Flags: superreview?(darin)
Attachment #219518 - Flags: superreview+
Attachment #219518 - Flags: review?(darin)
Attachment #219518 - Flags: review+
Comment on attachment 219518 [details] [diff] [review]
Probably a good idea.

Requesting branch approvals too; this is needed to make branch compile.  I'll merge to trunk and check in by a few hours from now.
Attachment #219518 - Flags: approval1.8.0.3?
Attachment #219518 - Flags: approval-branch-1.8.1?(darin)
Assignee: nobody → bzbarsky
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Attachment #219518 - Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+
Fixed on 1.8 branch.
Keywords: fixed1.8.1
(Reporter)

Comment 17

11 years ago
(In reply to comment #12)
> Created an attachment (id=219518) [edit]
> Probably a good idea.
> 
> Greg, would you mind testing this too?

Did a full rebuild with the latest patch overnight.  Works fine.  I'd say commit it ...

Comment on attachment 219518 [details] [diff] [review]
Probably a good idea.

approved for 1.8.0 branch, a=dveditz for drivers.
Attachment #219518 - Flags: approval1.8.0.3? → approval1.8.0.3+
Fixed on 1.8.0 branch.  Greg, thanks for the patch!
Keywords: fixed1.8.0.3
You need to log in before you can comment on or make changes to this bug.