The VMPI_spyCallback in SpyUtilPosix.cpp does this: RedirectLogOutput(SpyLog); MMgc::GCHeap::GetGCHeap()->DumpMemoryInfo(); RedirectLogOutput(NULL); Given that we have a RedirectLogOutput() function which could have been used to set a redirect log function previously, we should be restoring back the previous log function rather than setting it to NULL. The same issue is also present in other Spy utils.
Assignee: nobody → rulohani
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → flash10.x - Serrano
Created attachment 488931 [details] [diff] [review] Restore back the old log function
Attachment #488931 - Flags: superreview? → superreview?(lhansen)
I also had a thought that we can probably avoid having a new Get function for the loggging. RedirectLogOutput() can be modified to return the old log function which can then be used to reset it at the end. Though it might require the RedirectLogOutput to be renamed to something else.
Comment on attachment 488931 [details] [diff] [review] Restore back the old log function Tabstops in the file should be cleaned up.
Attachment #488931 - Flags: superreview?(lhansen) → superreview+
Created attachment 489338 [details] [diff] [review] Updated patch.
Comment on attachment 489338 [details] [diff] [review] Updated patch. Don't like the code duplication, please move the typedef, getter and logFunc to a common file. In general code in a platform specific file should be platform specific although and not a copy from other platform specific files (I realize other certain large code bases don't follow this rule but it would be nice to adhere to it in the vm).
Attachment #489338 - Flags: review?(treilly) → review-
RedirectLogOutput() also falls in the same duplication category then - common code in all platform specific files. Is there an existing common file where these could be moved?
(In reply to comment #6) > RedirectLogOutput() also falls in the same duplication category then - common > code in all platform specific files. Is there an existing common file where > these could be moved? I imagine if a file for it exists, it should be in vmbase/. But I do not see a good .cpp in there for this (that directory is a relatively new addition to the infrastructure).
Created attachment 501524 [details] [diff] [review] Patch v2 Take 2, with logging functions common to all platforms moved to a separate file.
Comment on attachment 501524 [details] [diff] [review] Patch v2 Looks good, my only concern is that GetCurrentLogFunction, LoggingFunction and RedirectLogOutput should be scoped somehow, ie put them in the vmbase namespace.
Created attachment 504805 [details] [diff] [review] Patch v3 Updated the scope of the log functions. I just used "using namespace vmbase;", let me know if I should instead just use the vmbase::RedirectlogOutput() etc. Also, the xcode and visual studio project files will need to be changed to add the 2 new files. Should those also be attached to the bug?
vmbase is not a dumping ground for code that's common to platform specific files. So far it has been a place to put code that must be shared between MMgc and AVMPlus because those are compiled separately by the Flash Player. As Tommy observes, we have no place (yet) to put code that's common in the various platform files. I'm not particularly allergic to code that's duplicated in the various VMPI implementation files, especially trivialities like these. SR+, but I'd like to solicit opinions from Steven and Edwin before it lands.
Attachment #504805 - Flags: superreview?(lhansen) → superreview+
Attachment #504805 - Flags: superreview+ → superreview?(edwsmith)
I generally agree with Lars, but IMHO the patch needs cleanup: One must-fix issue: declaring "extern LoggingFunction logFunc" in a cpp file is a recipe for pain down the road; extern declarations really *must* live in a header file. Style nits on this patch: -- We have a GetCurrentLogFunction(), so why do we also need to expose the global variable? -- if logFunc is going to be global, use the naming convention used elsewhere, "gLogFunc" or "g_logFunc".
OS: Mac OS X → Windows 7
Comment on attachment 504805 [details] [diff] [review] Patch v3 Agree with Lars + Steven. if this is trivial code then lets just duplicate it and not pollute vmbase. if its nontrivial then lets find a good place to share it between platforms. (/platform?)
Attachment #504805 - Flags: superreview?(edwsmith) → superreview-
Created attachment 557342 [details] [diff] [review] Patch v4 Moved the Logging methods to the GenericPortUtils.cpp in VMPI.
Attachment #557342 - Flags: superreview?(lhansen) → superreview+
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
changeset: 6559:8f002fb91ddd user: Ruchi Lohani<email@example.com> summary: Bug 592187 - Restore back the log function in VMPI_spyCallback (r=treilly, sr=lhansen) http://hg.mozilla.org/tamarin-redux/rev/8f002fb91ddd
You need to log in before you can comment on or make changes to this bug.