remove -fpascal-strings from mac build options

RESOLVED FIXED in mozilla6

Status

()

Core
Build Config
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: Usul, Assigned: Usul)

Tracking

Trunk
mozilla6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 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

12 years ago
Created attachment 212631 [details] [diff] [review]
remove -fpascal-string from build config

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

Comment 2

12 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

10 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

8 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

8 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

6 years ago
Created attachment 513055 [details] [diff] [review]
Hg version of the patch

(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

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

Comment 7

6 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

6 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

6 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

6 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 15

6 years ago
http://hg.mozilla.org/try/pushloghtml?changeset=bbf9f6e88788
(Assignee)

Comment 16

6 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

6 years ago
Depends on: 658218
(Assignee)

Comment 17

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

Updated

6 years ago
Blocks: 700519

Updated

6 years ago
Blocks: 700745
You need to log in before you can comment on or make changes to this bug.