224012 - VerReg.c warning: "VR_FILE_SEP" redefined

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[timeless]

/Users/timeless/mozilla/modules/libreg/src/VerReg.c
284:1: warning: "VR_FILE_SEP" redefined
278:1: warning: this is the location of the previous definition

Comment 1

[timeless]

Attachment: fix warning

Comment 2

[Daniel Veditz [:dveditz]]

Comment on attachment 134368 [details] [diff] [review]
fix warning

r=dveditz

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Doesn't this *change* what VR_FILE_SEP is for XP_MACOSX + XP_UNIX?

Comment 4

[timeless]

no it does not change XP_UNIX, but it does change XP_MACOSX. I think that the
change satisfies the spirit of the code. i can try to have someone confirm that
changing it does not break anything...

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I meant when both are defined, and it seems like XP_MACOSX may need "/".

Comment 6

[Andrew Thompson]

I think this change is bad. I think we want the seperator to be '/' on OS X.

Reasoning:

The seperator is used here:

http://lxr.mozilla.org/seamonkey/source/modules/libreg/src/VerReg.c#287

and that's called through here:
http://lxr.mozilla.org/seamonkey/source/modules/libreg/src/VerReg.c#555
http://lxr.mozilla.org/seamonkey/ident?i=vr_GetPathname

Tracing back we find one of the places the directory string is initialized is here:
http://lxr.mozilla.org/seamonkey/source/modules/libreg/src/reg.c#269

Which calls: FSRefMakePath, which is documented here:

http://developer.apple.com/documentation/Carbon/Reference/File_Manager/file_manager/function_group_8.html

as "returning paths suitable for POSIX-style calls". ie, it'll use '/'

So I think you need to rethink so the seperator on OS X remains '/'

Comment 7

[timeless]

Attachment: fix warning but keep behavior

Comment 8

[Andrew Thompson]

Is XP_UNIX defined for Mac OS X builds?

It might be clearer to write:

#elif defined(XP_UNIX) || defined(XP_BEOS) || defined(XP_MACOS_X)

Just so people less familar with OS X or the vagarities of Mozilla's #defines
realize that we're not taking the XP_MAC branch.

Comment 9

[timeless]

yeah it's defined. standard usage is: #if defined(XP_UNIX) && !defined(XP_MACOSX)
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/plugin/base/src/ns4xPlugin.cpp&rev=1.105&mark=469#445

Comment 10

[Andrew Thompson]

Apologies for the stream of questions, but is XP_MAC defined on OSX?
If it is you're going to get the wrong answer.

I still think it would be clearer is XP_MACOSX appeared in the if-then-else
explicitly. I'd rather the code said what it does than have to know that XP_MAC
is not defiend (counterintuitive) and XP_UNIX is defined (not immediately obvious).

Comment 11

[timeless]

nope, XP_MAC and XP_MACOSX are exclusive hence things like:
/embedding/browser/webBrowser/nsWebBrowser.cpp, line 70 -- #if (defined(XP_MAC)
|| defined(XP_MACOSX)) && !defined(MOZ_WIDGET_COCOA)

Comment 12

[Phil Ringnalda (:philor)]

Comment on attachment 141024 [details] [diff] [review]
fix warning but keep behavior

Conveniently enough, in the intervening five years XP_MAC has become undefined, so now you can do a simpler version without it (and r+sr? dveditz, because you're not getting anything out of lordpixel).

Comment 13

[Andrew Thompson]

Surprise!

Not entirely clear what you're proposing. What change do you believe should be made?

Comment 14

[Phil Ringnalda (:philor)]

You had your chance, in 2004. Too late now.

Comment 15

[Phil Ringnalda (:philor)]

Attachment: XP_MAC is long dead

Time to sweep it out, before the corpse starts to smell (any more).

Comment 16

[Phil Ringnalda (:philor)]

Comment on attachment 355521 [details] [diff] [review]
XP_MAC is long dead

Curse you, "failed to match any Bugzilla usernames."

Comment 17

[Andrew Thompson]

Your patch looks good.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 355521 [details] [diff] [review]
XP_MAC is long dead

r/sr=dveditz, but isn't libreg dead already?

Comment 19

[Phil Ringnalda (:philor)]

'e's not dead, 'e's just resting.

It's hypothetically used by migration, I think to hypothetically find Suite and 4.x profiles to import, which hypothetically still works even though we've started claiming it's unsupported despite the code still being around.

Comment 20

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/151cf0a020b0