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