Closed Bug 1063455 Opened 5 years ago Closed 5 years ago
Consider creating a define for MOZ
_STACKWALKING in configure .in
I had to add some #ifs around the use of NS_StackWalk for bug 1018966 and I noticed that the logic (which I copied :) ) that decides if nsStackTrace.cpp is built, is used in a couple of places. I think it makes sense to define something like MOZ_STACKWALKING to reproduce the logic at . This could then be used there and in the other places. It may even be useful for the other places that use NS_StackWalk even though they seem to have different logic protecting them at the moment.  http://dxr.mozilla.org/mozilla-central/source/xpcom/base/moz.build#125
Ben Smedberg didn't have a strong opinion either way on this. Do you think it is worth doing?
I am a fan of doing things Once And Only Once, yes. :)
This should make it easier for any new consumers of NS_StackWalk.
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
This should probably also get build peer review.
Comment on attachment 8485837 [details] [diff] [review] Define MOZ_STACKWALKING when NS_StackWalk is available and replace other instances of the same #if logic v1 Review of attachment 8485837 [details] [diff] [review]: ----------------------------------------------------------------- This is good stuff, but the configure.in checks need a build peer's review.
Comment on attachment 8485837 [details] [diff] [review] Define MOZ_STACKWALKING when NS_StackWalk is available and replace other instances of the same #if logic v1 >diff --git a/xpcom/build/LateWriteChecks.cpp b/xpcom/build/LateWriteChecks.cpp >--- a/xpcom/build/LateWriteChecks.cpp >+++ b/xpcom/build/LateWriteChecks.cpp >@@ -29,17 +29,17 @@ > #include <sys/stat.h> > #include <windows.h> > #else > #define NS_SLASH "/" > #endif > > #include "LateWriteChecks.h" > >-#if !defined(XP_WIN) || (!defined(MOZ_OPTIMIZE) || defined(MOZ_PROFILING) || defined(DEBUG)) >+#if defined(MOZ_STACKWALKING) You could just #ifdef if you want. Any idea if this #if is supposed to be the same as MOZ_STACKWALKING? (Or maybe !MOZ_STACKWALKING?) - http://mxr.mozilla.org/mozilla-central/source/accessible/base/Logging.cpp#70 It originally came from bug 733848 - based on the review comments there it sounds like folks weren't sure of the correct logic, maybe because we didn't have a consistent token for it :)
Attachment #8485837 - Flags: review?(mshal) → review+
Thanks both. Just kicked off another try run with a few actual tests running. (In reply to Michael Shal [:mshal] from comment #8) > Any idea if this #if is supposed to be the same as MOZ_STACKWALKING? (Or > maybe !MOZ_STACKWALKING?) - > http://mxr.mozilla.org/mozilla-central/source/accessible/base/Logging.cpp#70 > > It originally came from bug 733848 - based on the review comments there it > sounds like folks weren't sure of the correct logic, maybe because we didn't > have a consistent token for it :) Yeah, I thought about changing this to !MOZ_STACKWLAKING, but as this would turn the logging on unconditionally for non Windows builds, I decided against it. I've just raised bug 1065296, so there's more of a chance that this and the other uses of NS_StackWalk will get looked at. :)
Try push building on all platforms: https://tbpl.mozilla.org/?tree=Try&rev=1759dbca9add Even thought this is basically a build config change, a try push with some tests to be safe: https://tbpl.mozilla.org/?tree=Try&rev=73fb9c843c56 Thanks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff268a72378 FYI, for future reference, you can consolidate those two Try pushes into one by making use of the "Restrict tests to platform(s)" option on Trychooser :)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.