plevent code build breakage for OpenVMS

RESOLVED FIXED

Status

()

--
blocker
RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: colin, Assigned: colin)

Tracking

Trunk
DEC
OpenVMS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

18 years ago
Chris' recent plevent change has broken the OpenVMS build. I guess the OpenVMS 
specific changes were coded in a confusing way, but OpenVMS builds with both VMS 
and XP_UNIX defined.

Anyway, I have a patch and will attach next. Since this is platform specific I'd 
like to get it in to M0.81 if poss.
(Assignee)

Comment 1

18 years ago
Created attachment 27706 [details] [diff] [review]
Patch to xpcom/threads/plevent.c
Is that patch backwards or something?
(Assignee)

Comment 3

18 years ago
No, its the right way. I just need to add the two declarations to the VMS leg 
and then two lines of zeroizing to another VMS leg.

I moved the first VMS ifdef inside of the XP_UNIX ifdef to make it obvious that 
VMS is a subset of XP_UNIX. That's why it looks a little funny.
Oh, OK.  I'd much rather that you did something like:

#ifdef VMS
(..)
#endif /* VMS */
#ifdef XP_UNIX
(..)
#endif

I really don't like nested ifs.  They are hard to read and unless there's a
really good reason I'd rather not see them used.
(Assignee)

Comment 5

18 years ago
That's the ifdef structure I was using in plevent.c, and it bit me because you 
didn't realize that I needed all the XP_UNIX changes. I thought nested ifdef's 
would make that fact obvious (at the expense of a little readability, I admit).

But if you think its too gross I'll do it the old way.
Status: NEW → ASSIGNED
(Assignee)

Comment 6

18 years ago
Another reason for that particular set of ifdefs being nested is because there's 
a certain amount of the code that's common to VMS and XP_UNIX, and I wanted to 
emphasis this (as well as hopefully not breaking again the next time this code 
is changed). So instead of:

#ifdef VMS
a1
b
c
#endif /* VMS */
#ifdef XP_UNIX
a2
b
c
#endif

I have:

#ifdef XP_UNIX
#ifdef VMS
a1
#else
a2
#endif
b
c
#endif /* XP_UNIX */

Please let me know this morning which you prefer, as I really want to get this 
in before we branch.
In the future try and stay away from netsted comments, that's all.  Get this in
if you've got reviews from danm. r=blizzard

Comment 8

18 years ago
r=danm. sorry for the bustage. (I don't have sooper review status. Chris, wanna 
upgrade your r?)

The VMS ifdeffage in this file doesn't make it clear that it's dependent on 
XP_UNIX. Might want to add a comment while you're in there; maybe it'll help.
Yeah, you can use my r= any way you want.  I'm flexible.
(Assignee)

Comment 10

18 years ago
Checked in before the M0.81 branch. rev 1.19.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.