Closed
Bug 1047403
Opened 10 years ago
Closed 10 years ago
Printing std:stringstream requires a bunch of duplicated goop for android
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
Bug 1027496 buffered various debug output into std::stringstream before dumping it to stderr/logcat. Bug 1030245 fixed the issue where the output was getting truncated on android. However this required a bunch of duplicated code which is only really needed on android. We can refactor this to do better.
Assignee | ||
Comment 1•10 years ago
|
||
What I had in mind. Still building/testing this.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Attachment #8466201 -
Attachment description: WIP → Patch
Attachment #8466201 -
Flags: review?(nfroyd)
Attachment #8466201 -
Flags: review?(bgirard)
Comment 2•10 years ago
|
||
Comment on attachment 8466201 [details] [diff] [review]
Patch
Review of attachment 8466201 [details] [diff] [review]:
-----------------------------------------------------------------
::: xpcom/glue/nsDebug.h
@@ +13,5 @@
> #include "nsXPCOM.h"
> #include "mozilla/Assertions.h"
> #include "mozilla/Likely.h"
> #include <stdarg.h>
> +#include <sstream>
stringstream should be forward declared if possible, nsDebug.h is a common include. I think we can use '#include <iosfwd>' for this?
Attachment #8466201 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 3•10 years ago
|
||
Good point. I moved the sstream include to nsCRTGlue.cpp and used iosfwd in nsDebug.h.
Build-only try push:
https://tbpl.mozilla.org/?tree=Try&rev=09479402c191
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=09479402c191
Assignee | ||
Comment 4•10 years ago
|
||
Doesn't seem to like iosfwd that much.
New try with the original patch: https://tbpl.mozilla.org/?tree=Try&rev=41153b66c218
Comment 5•10 years ago
|
||
Comment on attachment 8466201 [details] [diff] [review]
Patch
Review of attachment 8466201 [details] [diff] [review]:
-----------------------------------------------------------------
I would really like to see <iosfwd> work, and your current try push seems to be blowing up. Can you investigate?
Attachment #8466201 -
Flags: review?(nfroyd)
Assignee | ||
Comment 6•10 years ago
|
||
My local linux build also will not find <iosfwd> for some reason. Although there are definitely files including it, because if I copy the bits of iosfwd that I need into nsDebug.h, I get a duplicate definition error and the compiler claims iosfwd was already included.
Also the try push shows OS X errors which will likely need a fix such as pointed to at https://bugzilla.mozilla.org/show_bug.cgi?id=965340#c9
Assignee | ||
Comment 7•10 years ago
|
||
Latest failed attempt: https://tbpl.mozilla.org/?tree=Try&rev=ec691928ba1b
Seems like STL requires infallible new, and xpcom/glue uses fallible new? At least in the context of the "sample program". I might just move this to LayersLogging.cpp or somewhere not so stringent with obscure requirements.
Assignee | ||
Comment 8•10 years ago
|
||
I moved it to LayersLogging since I got too frustrated with trying to put it in nsDebug.h. This works just fine:
https://tbpl.mozilla.org/?tree=Try&rev=45670c6023b3
Attachment #8466201 -
Attachment is obsolete: true
Attachment #8466662 -
Flags: review?(bgirard)
Updated•10 years ago
|
Attachment #8466662 -
Flags: review?(bgirard) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Keywords: checkin-needed
Comment 10•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•