428529 - Rename functions that conflict with definitions in windows.h

Status: RESOLVED INCOMPLETE

(Firefox Build System :: General, defect)

Description

[Benjamin Smedberg]

windows.h redefines a bunch of tokens which we use:

GetMessage
CreateMessage
LoadImage
ERROR
GetClassName

we have in the past tried to avoid including windows.h unless necessary, and use #undef to fix the remaining issues. This is going to become less and less feasible:

* MMgc picks up windows.h for inline functions we need
* #undefing the symbols doesn't work because we actually have to use some of the functions, or ATL headers use them, or semi-external code uses them

In bug 397929 we added the ability to rename the binary reflection of IDL signatures while keeping the script reflection intact. I'd like to use that technique to rename the above methods.

I've got a patch queue which does this now: I was not very consistent in the renaming, so I've currently got:

s/GetClassName/MozGetClassName/
s/LoadImage/_LoadImage/
#undef ERROR (because this is a constant and not a method, we can't rename it in XPIDL the same way).

Do you (dbaron/jst/pav) have a preference about Moz as a prefix or suffix or an underscore prefix or suffix? Automatically munging my patches for different renaming styles should be relatively painless.

Comment 1

[Benjamin Smedberg]

The current patches should show up at http://hg.mozilla.org/users/bsmedberg_mozilla.com/windows-redefine-patches/ but there seems to be a delay right now.

Comment 2

[Stuart Parmenter]

I don't think we should prefix our function names.  Windows #defines a _lot_ of stuff and we should really not let it screw us up.  Perhaps we should early #include it and rename its functions?

Comment 3

[Benjamin Smedberg]

The problem is that other headers (ATL headers), as well as our own code (widget/) actually do want the Windows functions. In our code we can just use GetMessageW explicitly, but what do we do about ATL or other external dependencies?

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

ATL/external code should be #including windows.h; we should not be #including windows.h globally.  Whatever functions mmgc needs we need to just isolate out of windows.h (or, possibly, #include the specific windows header that has those pieces, instead of taking windows.h).

Comment 5

[Benjamin Smedberg]

I don't know whether that would be acceptable to MMgc: treilly, thoughts? I think we only need winbase.h instead of the full windows.h for MMgc itself, but that may mean that avmcore and the flash player would need to #include <windows.h> in other places.

Comment 6

[Tommy Reilly]

Norrowing the includes in MMgc is fine with me, ideally only the .cpp module that needed the windows defs would include it and MMgc.h wouldn't.  

Comment 7

[Shawn Wilsher :sdwilsh]

Is it a compile time conflict or a linker conflict that we have (or both)?  Couldn't we use a namespace for mozilla "stuff" to fix that issue without renaming methods?

Comment 8

[Jeff Walden [:Waldo]]

The preprocessor runs far before namespace resolution occurs -- it's just processing a stream of tokens, not names at all.  I'm pretty sure either renaming or macro pushing/popping (which then must happen before and after every use of the function) are the only ways to solve this, aside from Microsoft breaking backwards compatibility.

Re comment 1, I tend to think a Moz suffix is good because it doesn't disrupt autocompletion since you don't have to get over the hump of a Moz to get to the autocompletable stuff.  Also, that's more consistent considering getters and setters already start with Get and Set, I think.

Comment 9

[Benjamin Smedberg]

Update: I tried to compile Tamarin with <winbase.h> instead of <windows.h>. I had to also #include <windef.h>. This doesn't work because windows.h sets up architecture defines which the rest of the windows headers use. I can get everything to compile if I define _X86_ as well... are we ok with manually defining _X86_ in both the Tamarin and Mozilla build systems to avoid the windows.h dependency?

Comment 10

[Tommy Reilly]

That makes me quesy,  I would be happy if windows.h was only included in the .cpp files that needed it and not included in any .h files that get included from MMgc.h.   How's that for a consolation prize?

Comment 11

[Benjamin Smedberg]

I don't think that will work, because the header files are calling Windows functions that need to be declared, such as InterlockedCompareExchange at http://hg.mozilla.org/tamarin-central/index.cgi/file/548a350c9c29/MMgc/GCSpinLockWin.h#l68 or HeapAlloc at http://hg.mozilla.org/tamarin-central/index.cgi/file/548a350c9c29/MMgc/GCAllocObjectWin.cpp#l47

Comment 12

[Tommy Reilly]

The impl doesn't have to be in the header.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It does if you want (reliable) inlining, no?

Why don't we just put the declarations of the necessary windows methods (InterlockedCompareExchange, HeapAlloc, any others) in a local winsupport.h, and just include that?  For bonus points, could add a winsupport.cpp file that just #includes windows.h and winsupport.h, to make sure that there are no declaration conflicts.

Comment 14

[Benjamin Smedberg]

Not needed at the moment, closing out things which are probably dead.