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

RESOLVED FIXED in mozilla1.9beta4

Status

()

RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: benjamin, Assigned: Waldo)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9beta4
x86
Windows XP
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
Created attachment 282734 [details] [diff] [review]
Damn MS, rev. 1

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)
(Assignee)

Comment 1

11 years ago
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
(Assignee)

Comment 2

11 years ago
Created attachment 301449 [details] [diff] [review]
Patch

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)
(Assignee)

Comment 3

11 years ago
I don't see a reason to bump the iid since the patch preserves binary compatibility.
Blocks: 387706
Status: NEW → ASSIGNED
Version: unspecified → Trunk
(Reporter)

Comment 4

11 years ago
Comment on attachment 301449 [details] [diff] [review]
Patch

Beautiful.
Attachment #301449 - Flags: review?(benjamin) → review+
(Assignee)

Comment 5

11 years ago
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?

Updated

11 years ago
Attachment #301449 - Flags: approval1.9? → approval1.9+
(Reporter)

Updated

11 years ago
Keywords: checkin-needed
(Assignee)

Comment 6

11 years ago
I'll pick this up later today, most likely tonight.
Keywords: checkin-needed
(Assignee)

Comment 7

11 years ago
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
Keywords: dev-doc-complete
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
(Assignee)

Comment 8

11 years ago
Created attachment 302036 [details] [diff] [review]
Add binaryname(MessageMoz) to nsIConsoleMessage.message
Attachment #302036 - Flags: review?(benjamin)
(Reporter)

Updated

11 years ago
Attachment #302036 - Flags: review?(benjamin) → review+
(Assignee)

Comment 9

11 years ago
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.
(Assignee)

Comment 10

11 years ago
Created attachment 302735 [details] [diff] [review]
Builds on Windows tryserver
Attachment #302036 - Attachment is obsolete: true
Attachment #302735 - Flags: review+
Attachment #302735 - Flags: approval1.9?

Updated

11 years ago
Attachment #302735 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 11

11 years ago
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
Last Resolved: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Comment 12

11 years ago
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.
(Reporter)

Comment 13

11 years ago
Until we start having compiler errors, don't worry about it.

Comment 14

11 years ago
(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.

Comment 16

11 years ago
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...

Updated

11 years ago
Component: xpidl → XPCOM
QA Contact: xpidl → xpcom
You need to log in before you can comment on or make changes to this bug.