Closed
Bug 394447
Opened 17 years ago
Closed 17 years ago
Tamarin's __DEBUGGING__ macro conflicts with Mac system headers
Categories
(Tamarin Graveyard :: Virtual Machine, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: jorendorff, Assigned: jorendorff)
Details
Attachments
(2 files)
(deleted),
patch
|
dansmith
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dansmith
:
review+
|
Details | Diff | Splinter Review |
(filing this bug against Tamarin VM, but it's really against the Tamarin build system)
The Tamarin build system defines __DEBUGGING__ via a command-line option to the compiler. It doesn't appear to be used anywhere, though.
We should remove it; there's a Mac OS X system header, CarbonCore/Debugging.h, which contains this code:
#ifndef __DEBUGGING__
#define __DEBUGGING__
...
#endif /* __DEBUGGING__ */
If __DEBUGGING__ is defined on the command line, you basically can't include this header. This actually tripped me up doing build integration for ActionMonkey.
Assignee | ||
Comment 1•17 years ago
|
||
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•17 years ago
|
||
It appears __DEBUGGING__ was added intentionally to disable that header, due to a naming conflict between pcre and Carbon. So the attached patch won't work. Maybe we can fix this some other way.
Assignee | ||
Comment 3•17 years ago
|
||
This conflict between pcre and Carbon is fixed in the latest release of pcre, version 7.3. We're using version 6.0. Any reason not to upgrade?
Comment 4•17 years ago
|
||
Not that I can think of.
Jason, the PCRE in Tamarin is a modified version to make it compatible with ECMAScript. Here's a note from Chris Nuuja who made the changes to the Tamarin PCRE:
I've made lots of small mods and one or two significant mods. ECMAScript RegExp differs from the pcre standard in a few significant ways. There are lots of expressions which will evaluate differently between the two.
If the new changes don't affect the code I moded, then it should be simple. Look at the change history for pcre_compile.cpp and pcre_exec.cpp (look in mainline flashPlayer/avmplus/pcre. For some reason /as/avmplus/pcre is missing a lot of change history).
Assignee | ||
Comment 6•17 years ago
|
||
In that case-- Since it is a modified version of PCRE anwyay, there's no harm manually patching in the few lines I need.
+It turns out that the Mac Debugging.h header also defines the macro DPRINTF, so
+be absolutely sure we get our version. */
+
+#undef DPRINTF
I'll post a new patch with that change. (If you want to upgrade to PCRE 7.3 anyway, let me know.)
Comment 7•17 years ago
|
||
Sounds like we should submit our changes (with suitable ifdef's) back to the PCRE maintainers, so we can track the current version and not have to re-make them...
Assignee | ||
Comment 8•17 years ago
|
||
Here's the fix. (Apply both patches.)
For some reason Bugzilla is not showing me controls for requesting a code review. So, this is me requesting a review. :-) Anyone?
Assignee | ||
Updated•17 years ago
|
Attachment #279104 -
Flags: review?(dansmith)
Assignee | ||
Updated•17 years ago
|
Attachment #279114 -
Flags: review?(dansmith)
Attachment #279114 -
Flags: review?(dansmith) → review+
Attachment #279104 -
Flags: review?(dansmith) → review+
Assignee | ||
Comment 9•17 years ago
|
||
pushed changesets d81eb3457771 and e80d62e610e4.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•