Closed Bug 397929 Opened 13 years ago Closed 13 years ago

Allow changing the C++ method names generated for attributes or methods to avoid naming conflicts

Categories

(Core :: XPCOM, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: benjamin, Assigned: Waldo)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

Attached patch Damn MS, rev. 1 (obsolete) — Splinter Review
This is for moz2:

windows.h ends up redefining a bunch of very common tokens such as

GetMessage
ERROR
CompareString
GetClassName

Because MMgc.h ends up #including <windows.h> we end up redefining these a lot in places where we previously didn't. Some of these places can be fixed with a simple #undef, but GetMessage in particular can't: GetMessage is used in ATL headers and must not be undefined. So I've hacked XPIDL to emit a different binary method name for that particular case.

I didn't rename the signature in nsIConsoleMessage because 1) that's hard to get right with all the JS callers and 2) there appear to be places which expect JS exception .message to match the .message of nsIScriptError->nsIConsoleMessage
Attachment #282734 - Flags: superreview?(shaver)
Attachment #282734 - Flags: review?(shaver)
I think I may have a principled fix; it works on my one testcase so far.  I'm rebuilding now to check...
Assignee: benjamin → jwalden+bmo
Attached patch PatchSplinter Review
Should be simple enough to understand...just add binaryname(Name) to the bracketed attributes for a method or attribute, and then you either use Class::Name or [GS]etName to define the method.  This patch works for me with postMessage, and it worked by visual inspection of the generated header on the following attribute in an IDL interface:

  [binaryname(MessageMoz)] attribute AString message;
Attachment #282734 - Attachment is obsolete: true
Attachment #301449 - Flags: review?(benjamin)
Attachment #282734 - Flags: superreview?(shaver)
Attachment #282734 - Flags: review?(shaver)
I don't see a reason to bump the iid since the patch preserves binary compatibility.
Blocks: postMessage
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Comment on attachment 301449 [details] [diff] [review]
Patch

Beautiful.
Attachment #301449 - Flags: review?(benjamin) → review+
Comment on attachment 301449 [details] [diff] [review]
Patch

(XPCOM doesn't require sr, just in case you, Mr. Anonymous Approval-Granter, didn't know.  :-) )
Attachment #301449 - Flags: approval1.9?
Attachment #301449 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
I'll pick this up later today, most likely tonight.
Keywords: checkin-needed
I documented this in:

http://www.mozilla.org/scriptable/xpidl/idl-authors-guide/rules.html

I think we should leave renaming individual names like className, message, etc. to followup bugs, for the most part, but I'd like to add this to at least one attribute so it's demonstrated that the attribute stuff works.  Patch for nsIConsoleMessage.message to do this in a sec...
Component: XPCOM → xpidl
QA Contact: xpcom → xpidl
Summary: GetMessage redefined by windows.h used in nsIConsoleMessage, and other redefinitions that hurt → Allow changing the C++ method names generated for attributes or methods to avoid naming conflicts
Target Milestone: --- → mozilla1.9beta4
Attachment #302036 - Flags: review?(benjamin)
Attachment #302036 - Flags: review?(benjamin) → review+
For the record, I'm trying to fight tryserver into verifying that Windows will run with this patch, particularly with the #undef GetMessage hunk of it.  It's been slow going due to lots of recent tryserver bustage (I'm on my second bug report to get the Windows one working).  I'll land once I get verification that nothing else depends on that #undef-inition.
Attachment #302036 - Attachment is obsolete: true
Attachment #302735 - Flags: review+
Attachment #302735 - Flags: approval1.9?
Attachment #302735 - Flags: approval1.9? → approval1.9+
In on trunk.  Now that both methods and attributes are being used here, I'm going to mark this FIXED; further cleanup of Windows's mess can go in other bugs.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
How do I know if I need to put this [binaryname(MessageMoz)] thing in a .idl or not?  For example, webservices has a message attribute in nsISOAPMessage.idl but there are no undef GetMessage's anywhere in the webservices code.  And it returns a nsIDOMDocument, not a string.
Until we start having compiler errors, don't worry about it.
(In reply to comment #13)
> Until we start having compiler errors, don't worry about it.
> 

Well, reed changed nsSOAPMessage in webservices to have GetMessageMoz which caused compile errors because he missed some spots.  So I was trying determine if his change was even needed.  Since webservices compiled fine without his change, I'll probably back it out.  But I will still need to fix webservices proxy because has a class that extends nsIException which did change to use GetMessageMoz.  I'll open a bug on webservices to make these changes.
(In reply to comment #14)
> Well, reed changed nsSOAPMessage in webservices to have GetMessageMoz which
> caused compile errors because he missed some spots.  So I was trying determine
> if his change was even needed.  Since webservices compiled fine without his
> change, I'll probably back it out.

This was why I changed it.

<Ubulette> [reed], trunk is broken http://paste.ubuntu.com/4517/

...I make change...

<Ubulette> [reed], still broken
<Ubulette> nsSOAPMessage.cpp:68: error: no 'nsresult nsSOAPMessage::GetMessageMoz(nsIDOMDocument**)' member function declared in class 'nsSOAPMessage'
<Ubulette> before, it was
<Ubulette> nsSOAPException.cpp:63: error: no 'nsresult nsSOAPException::GetMessage(char**)' member function declared in class 'nsSOAPException'
<Ubulette> http://paste.ubuntu.com/4525/

So, webservices does need some change somewhere, it seems.
Thanks Reed, that helps.  I see why nsSOAPException would give an error, but not nsSOAPMessage.  Maybe it is a platform thing.  I guess I'll leave nsSOAPMessage alone and just add what I need to get my windows machine building again.
So...  Over here, I'm not seeing any GetMessageMoz in the generated nsISoapMessage.h.  On the other hand, the C++ was changed to use GetMessageMoz, so the whole thing can't possibly compile...

Anyone care to shed some light on how the heck this is supposed to work?
Hmm.  Looks like I'd somehow managed to pick up the C++ changes but not the idl changes...  repulling tip from toplevel made it work...
Component: xpidl → XPCOM
QA Contact: xpidl → xpcom
You need to log in before you can comment on or make changes to this bug.