Closed Bug 56767 Opened 25 years ago Closed 22 years ago

XP_PC a misnomer. Should be XP_WIN32

Categories

(SeaMonkey :: Build Config, defect, P3)

x86
Windows 2000
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.4beta

People

(Reporter: cls, Assigned: leaf)

Details

Attachments

(13 files)

577 bytes, patch
Details | Diff | Splinter Review
7.42 KB, patch
Details | Diff | Splinter Review
5.65 KB, patch
Details | Diff | Splinter Review
5.78 KB, patch
Details | Diff | Splinter Review
22.51 KB, patch
Details | Diff | Splinter Review
1.34 KB, patch
Details | Diff | Splinter Review
2.81 KB, patch
Details | Diff | Splinter Review
2.84 KB, patch
Details | Diff | Splinter Review
4.51 KB, patch
Details | Diff | Splinter Review
1.00 KB, patch
Details | Diff | Splinter Review
1.29 KB, patch
Details | Diff | Splinter Review
1.09 KB, patch
Details | Diff | Splinter Review
76.02 KB, patch
mkaply
: review+
Details | Diff | Splinter Review
mkaply has brought this up a number of times and timeless & I just went over it again on irc. For some reason (mkaply can fill in the details), XP_PC is used on platforms other than MS Windows, yet every developers continue to use XP_PC as a win32 exculsive define. To start with, we need to add -DXP_WIN32 to config/config.mak so that it is always defined instead of requiring xp_core.h. That's a 30sec process. Then we need to scrub XP_PC from the source trees. That should eat up a couple of hours. Once that's done, we should be able to remove XP_PC from config/config.mak and listen to the develoeprs wonder why their new code breaks.
In a number of cases, XP_PC is used for both because it relates to "Intel" type things, or things that are consistent between OS/2 and Windows. Another note - I believe there should be XP_WIN and XP_WIN32. XP_WIN for stuff that is generic windows (like include windows.h for instance) and XP_WIN32 for WIN32 specific stuff and probably eventually XP_WIN64. I'll be more than happy to help with this change next week :)
"Intel" sort of things? Isn't that what -D_X86_ is for? So does OS/2 use any code that falls under XP_WIN?
Specific examples of XP_PC between OS/2 and Windows DLL names: http://lxr.mozilla.org/seamonkey/source/editor/base/nsEditor.cpp#124 File system stuff (back slashes) http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3102 Other stuff that I don't know: http://lxr.mozilla.org/seamonkey/source/js/src/jsfile.c#264 URl conversion: http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsStdURL.cpp#983 Context menus: http://lxr.mozilla.org/seamonkey/source/rdf/content/src/nsXULPopupListener.cpp#1 94 etc. By the way, if you search on XP_PC in lxr - every line that has the !defined XP_OS2 can be fixed because of this change. Very nice.
Ok, patch has been checked in. As for the next step....mkaply, are you saying that we should leave XP_PC or replace it with XP_WIN (or XP_DOS)?
I think for step one I'd like to remove a lot if the !defined XP_OS2 and elif XP_PC. I'll put together a patch in the next couple of days. Once we get rid of the "Windows" XP_PC stuff then we can decide what to really call XP_PC. Note by the way that I'll be moving stuff around in these diffs. Example: #ifdef XP_OS2 #elif defined XP_PC #elif defined XP_UNIX #endif I only had to put XP_OS2 at the beginning because of the XP_PC. Should be: #ifdef XP_WIN #elif defined XP_UNIX #elif defined XP_OS2 #endif
The patches look good to me. r=cls
Who would be the sr/a= on these? leaf I assume?
yes, he should be back tomorrow.
ahem. @@ -531,7 +531,7 @@ #ifdef NS_EXPLICIT_FUNC_TEMPLATE_ARG if ((conv=use_facet<ofacet_type>(getloc()). #elif defined(XP_PC) - if ((conv=use_facet(getloc(), (ofacet_type*)0, false). + if ((conv=use_facet(getloc(), (ofacet_type*)0, false). #else if ((conv=use_facet(getloc(), (ofacet_type*)0). #endif am i an idiot, or are these if statements having `(' matched with `.'? (patch 17620)
Nope, just a fine example of one of the worst #ifdefs I have ever seen in my life: Start reading at: http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsStdFileStream.h#531 And please make sure you are sitting down.
r=leaf with the fixed paren patch...my eyes are bleeding after reading that code. Thanks, mike.
Note from brendan giving approval: Michael Kaply wrote: > Hey guys, > > The changes for bug: > > http://bugzilla.mozilla.org/show_bug.cgi?id=56767 > > impact lots of areas, so we were going to have shaver sr= the changes > rather than going to each component owner. > > He hasn't really been responsive. Shaver's been busy with other stuff lately, and isn't the best fit here anyway. > So question is, can cls act as a super reviewer on this stuff? Or do one of > you want to do it? > > I'm getting both leaf and cls to look at all the changes I make. I think you're done, per http://www.mozilla.org/hacking/reviewers.html#exceptions . /be
Attached patch morkConfig.hSplinter Review
Does this bug have any relationship to bug #65727 (currently assigned to dprice)?
65727 is a side effect of this work. When we added XP_WIN to the build, we didn't add it to component builds. So when I started changing XPCOM to use XP_WIN, components were still using XP_PC. 65727 is asking for an #ifdef somewhere in a global headers that #errors if XP_PC is defined without XP_WIN or XP_OS2. This is probably a really good idea. Note that although this bug hasn't been touched in a while, it is still being worked. I've just been busy.
Target Milestone: --- → mozilla1.2
bug cleanup - all leaf's bugzilla bugs should be assigned to leaf@mozilla.org (not leaf@netscape.com), now and any future bugs created. this should be a one time change, apologies for the spam.
Assignee: leaf → leaf
Attached patch v1.0Splinter Review
* Fixes the remaining XP_PC ifdefs for the entire tree except NSPR, NSS & JS (see bug 74999 for the last one)
Attachment #119034 - Flags: superreview?(alecf)
Attachment #119034 - Flags: review?(mkaply)
Comment on attachment 119034 [details] [diff] [review] v1.0 good riddens! sr=alecf
Attachment #119034 - Flags: superreview?(alecf) → superreview+
Comment on attachment 119034 [details] [diff] [review] v1.0 agreed. lets get rid of these
Attachment #119034 - Flags: review?(mkaply) → review+
All gone. (All that count anyway.)
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.2alpha → mozilla1.4beta
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: