remove -fpascal-strings from mac build options

RESOLVED FIXED in mozilla6

Status

defect
RESOLVED FIXED
14 years ago
Last year

People

(Reporter: Usul, Assigned: Usul)

Tracking

Trunk
mozilla6
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

14 years ago
We pass a flag to gcc so it can recognize and use pascal strings. This is now discouraged by Apple (see url). We don't seem to use pascal strings. Hence I'm opening this bug so we don't pass this flag to gcc.
Assignee

Comment 1

14 years ago
I've successfully build Camino with pacth applied.
Asking Mento to review.
Attachment #212631 - Flags: review?(mark)

Comment 2

14 years ago
Comment on attachment 212631 [details] [diff] [review]
remove -fpascal-string from build config

I could swear that we're still using "\p" in places where breakage might not be all that apparent.  If you want to kill -fpascal-strings, you'll need to kill that code too.

I see one use of "\p" in nsMacResources.cpp, for example.
Attachment #212631 - Flags: review?(mark) → review-

Comment 3

12 years ago
I just grepped the current trunk for '"\\p' and looked through the results, filtering out code that's not part of any current build as far as I know.  The following uses of "\p" still need to be removed:

modules/plugin/samples/default/mac/npmac.cpp
xpcom/io/nsAppFileLocationProvider.cpp
xpfe/bootstrap/appleevents/nsAEApplicationClass.cpp
xpfe/bootstrap/appleevents/nsAEDefs.h
xpfe/bootstrap/appleevents/nsMacUtils.cpp
xpinstall/src/nsInstallFileOpItem.cpp

All other uses of "\p" are either in files that are never built or are in sections of files that are never built.  For example, there's still some code only used in pre-Mac OS X or CFM builds that are no longer of any consequence to us.  Not all of the files above are always built.  xpinstall, for one, is frequently not built, but it should be fixed because as far as I know it hasn't yet been formally deprecated and it's still possible to build it.

Some of these are just DebugStr only in debug builds, and can be converted to some other form of logging or just eliminated.  Some just use "\p" to initialize empty Pascal strings (Str255 s = "\p";), which can also be done with {0} (Str255 s = {0};).  Some are still doing things with resources that probably no longer work.

These should all be easy to eliminate.

Comment 4

10 years ago
If this is fixed it would perhaps be possible to compile firefox with a vanilla gcc on OSX. It currently fails because -fpascal-strings is in Apple's gcc only.
Assignee

Comment 5

10 years ago
(In reply to comment #4)
> If this is fixed it would perhaps be possible to compile firefox with a vanilla
> gcc on OSX. It currently fails because -fpascal-strings is in Apple's gcc only.

Unfortunatly I got lazy and never did the proper patch. Feel free to remove the pascal strings in the code.
Assignee

Comment 6

8 years ago
(In reply to comment #3)
> I just grepped the current trunk for '"\\p' and looked through the results,
> filtering out code that's not part of any current build as far as I know.  The
> following uses of "\p" still need to be removed:
> 
> modules/plugin/samples/default/mac/npmac.cpp
> xpcom/io/nsAppFileLocationProvider.cpp
> xpfe/bootstrap/appleevents/nsAEApplicationClass.cpp
> xpfe/bootstrap/appleevents/nsAEDefs.h
> xpfe/bootstrap/appleevents/nsMacUtils.cpp
> xpinstall/src/nsInstallFileOpItem.cpp

When I was able to find the file in mxr - I didn't find any occurrences of \p anymore in them.

> All other uses of "\p" are either in files that are never built or are in
> sections of files that are never built.  For example, there's still some code
> only used in pre-Mac OS X or CFM builds that are no longer of any consequence
> to us.  Not all of the files above are always built.  xpinstall, for one, is
> frequently not built, but it should be fixed because as far as I know it hasn't
> yet been formally deprecated and it's still possible to build it.
> 

I also grepped my local copy of comm-central and nothing poped out.
Assignee: nobody → ludovic
Attachment #212631 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #513055 - Flags: review?(ted.mielczarek)
Assignee

Updated

8 years ago
Hardware: PowerPC → All
Attachment #513055 - Flags: review?(ted.mielczarek) → review+
Assignee

Comment 7

8 years ago
Comment on attachment 513055 [details] [diff] [review]
Hg version of the patch

Requesting approval. removing unused code.
Attachment #513055 - Flags: approval2.0?
Comment on attachment 513055 [details] [diff] [review]
Hg version of the patch

Nope, into risk reduction, so no.
Attachment #513055 - Flags: approval2.0? → approval2.0-
Feel free to land this on the build-system branch.

Comment 10

8 years ago
(In reply to comment #6)
> Created attachment 513055 [details] [diff] [review]
> Hg version of the patch

There is the same -fpascal-strings in /js/src/configure.in, is it also possible to remove it from there?
OS: Mac OS X → All
Checked into build-system repo:

http://hg.mozilla.org/projects/build-system/rev/71c17252918a
Whiteboard: fixed-in-bs
Assignee

Comment 12

8 years ago
(In reply to comment #11)
> Checked into build-system repo:
> 
> http://hg.mozilla.org/projects/build-system/rev/71c17252918a

Which burns on /builds/slave/bld-system-osx-dbg/build/browser/components/migration/src/nsMacIEProfileMigrator.cpp:155
Assignee

Updated

8 years ago
Whiteboard: fixed-in-bs
browser/components/migration/src/nsMacIEProfileMigrator.cpp was removed in bug 656205.  Does the patch still cause burning on a current tree in build-system?
Feel free to get someone to push this to the try server to check.
Assignee

Comment 16

8 years ago
(In reply to comment #13)
> browser/components/migration/src/nsMacIEProfileMigrator.cpp was removed in
> bug 656205.  

It does because of components/migration/src/nsSafariProfileMigrator.cpp:1285: error: unknown escape sequence '\p'
Assignee

Updated

8 years ago
Depends on: 658218
Assignee

Comment 17

8 years ago
Got a green build !!!
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/b6202b4a98f1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Blocks: 700519

Updated

8 years ago
Blocks: 700745

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.