Closed
Bug 56767
Opened 24 years ago
Closed 21 years ago
XP_PC a misnomer. Should be XP_WIN32
Categories
(SeaMonkey :: Build Config, defect, P3)
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+
alecf
:
superreview+
|
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.
Comment 1•24 years ago
|
||
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?
Comment 4•24 years ago
|
||
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)?
Comment 6•24 years ago
|
||
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
Comment 7•24 years ago
|
||
Comment 8•24 years ago
|
||
Comment 9•24 years ago
|
||
Comment 10•24 years ago
|
||
Comment 11•24 years ago
|
||
Comment 12•24 years ago
|
||
Reporter | ||
Comment 13•24 years ago
|
||
The patches look good to me. r=cls
Comment 14•24 years ago
|
||
Who would be the sr/a= on these? leaf I assume?
Comment 15•24 years ago
|
||
yes, he should be back tomorrow.
Comment 16•24 years ago
|
||
Assignee | ||
Comment 17•24 years ago
|
||
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)
Comment 18•24 years ago
|
||
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.
Assignee | ||
Comment 19•24 years ago
|
||
r=leaf with the fixed paren patch...my eyes are bleeding after reading that code. Thanks, mike.
Comment 20•24 years ago
|
||
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
Comment 21•24 years ago
|
||
Comment 22•24 years ago
|
||
Comment 23•24 years ago
|
||
Comment 24•24 years ago
|
||
Comment 25•24 years ago
|
||
Does this bug have any relationship to bug #65727 (currently assigned to dprice)?
Comment 26•24 years ago
|
||
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.
Updated•23 years ago
|
Target Milestone: --- → mozilla1.2
Comment 27•22 years ago
|
||
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
Comment 28•21 years ago
|
||
* Fixes the remaining XP_PC ifdefs for the entire tree except NSPR, NSS & JS (see bug 74999 for the last one)
Updated•21 years ago
|
Attachment #119034 -
Flags: superreview?(alecf)
Attachment #119034 -
Flags: review?(mkaply)
Comment 29•21 years ago
|
||
Comment on attachment 119034 [details] [diff] [review] v1.0 good riddens! sr=alecf
Attachment #119034 -
Flags: superreview?(alecf) → superreview+
Comment 30•21 years ago
|
||
Comment on attachment 119034 [details] [diff] [review] v1.0 agreed. lets get rid of these
Attachment #119034 -
Flags: review?(mkaply) → review+
Comment 31•21 years ago
|
||
All gone. (All that count anyway.)
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.2alpha → mozilla1.4beta
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•