RESOLVED FIXED 119636
dataLog dumpers for WebCore's basic geometry types
https://bugs.webkit.org/show_bug.cgi?id=119636
Summary dataLog dumpers for WebCore's basic geometry types
Tim Horton
Reported 2013-08-09 12:56:05 PDT
This way you can dataLog(anIntRect, someFloatPoint, "\n") and all will be well. Will save (Simon and I at least) lots of typing.
Attachments
take one (6.79 KB, patch)
2013-08-09 14:10 PDT, Tim Horton
fpizlo: review+
eflews.bot: commit-queue-
Tim Horton
Comment 1 2013-08-09 14:10:15 PDT
Created attachment 208452 [details] take one I'm somewhat tempted to put them all in one big implementation file (GeometryFormatters? I have no idea) instead of spreading datalog stuff throughout all these classes (especially because of IntPoint and IntSize lacking implementation files causing PrintStream.h inclusion in their headers -- but maybe PrintStream.h doesn't change often and that won't be a problem?).
Darin Adler
Comment 2 2013-08-09 14:27:06 PDT
Comment on attachment 208452 [details] take one What’s the rationale for using member functions for these? Easier to invoke from the debugger?
Darin Adler
Comment 3 2013-08-09 14:28:02 PDT
(In reply to comment #1) > especially because of IntPoint and IntSize lacking implementation files causing PrintStream.h inclusion in their headers Make IntPoint.cpp and IntSize.cpp instead; what’s the argument against that?
Tim Horton
Comment 4 2013-08-09 14:32:05 PDT
(In reply to comment #3) > (In reply to comment #1) > > especially because of IntPoint and IntSize lacking implementation files causing PrintStream.h inclusion in their headers > > Make IntPoint.cpp and IntSize.cpp instead; what’s the argument against that? Valid point :) (In reply to comment #2) > (From update of attachment 208452 [details]) > What’s the rationale for using member functions for these? Easier to invoke from the debugger? Is there a better way? It looked to me like DataLog falls back on calling T::dump() when it doesn't know about the type. Maybe pizlo can comment.
Tim Horton
Comment 5 2013-08-09 14:34:07 PDT
(In reply to comment #2) > (From update of attachment 208452 [details]) > What’s the rationale for using member functions for these? Easier to invoke from the debugger? Also, you don't invoke these manually; they get invoked via dataLog()
Darin Adler
Comment 6 2013-08-09 14:35:08 PDT
(In reply to comment #4) > (In reply to comment #2) > > What’s the rationale for using member functions for these? Easier to invoke from the debugger? > > Is there a better way? It looked to me like DataLog falls back on calling T::dump() when it doesn't know about the type. Maybe pizlo can comment. We should fix DataLog so it calls dump(const X&), which in turn can be a template that calls X::dump. That way you can make something dump-able without changing the class itself.
EFL EWS Bot
Comment 7 2013-08-09 14:49:31 PDT
Comment on attachment 208452 [details] take one Attachment 208452 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1359579
Filip Pizlo
Comment 8 2013-08-09 15:29:38 PDT
(In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #2) > > > What’s the rationale for using member functions for these? Easier to invoke from the debugger? > > > > Is there a better way? It looked to me like DataLog falls back on calling T::dump() when it doesn't know about the type. Maybe pizlo can comment. > > We should fix DataLog so it calls dump(const X&), which in turn can be a template that calls X::dump. That way you can make something dump-able without changing the class itself. This already works. The canonical way to make something dump able is to add a dump(PrintStream&) method. But you can also add a WTF::printInternal(PrintStream&, const T&) function and get the same effect. In JSC the idiom I ultimately converged to was preferring a dump method over a printInternal function, except in the case of enums (obviously). I don't have a good reason for that preference other than it usually leads to less code, since a function in WTF means opening and then closing that namespace, and having to use whatever namespace T is in (or have a lot of JSC::T like code).
Filip Pizlo
Comment 9 2013-08-09 15:31:34 PDT
Comment on attachment 208452 [details] take one View in context: https://bugs.webkit.org/attachment.cgi?id=208452&action=review > Source/WebCore/platform/graphics/IntRect.h:211 > + void dump(WTF::PrintStream& out) const; Don't need to say "WTF::"
Filip Pizlo
Comment 10 2013-08-09 15:33:17 PDT
(In reply to comment #1) > Created an attachment (id=208452) [details] > take one > > I'm somewhat tempted to put them all in one big implementation file (GeometryFormatters? I have no idea) instead of spreading datalog stuff throughout all these classes (especially because of IntPoint and IntSize lacking implementation files causing PrintStream.h inclusion in their headers -- but maybe PrintStream.h doesn't change often and that won't be a problem?). It's already the case that a bunch of JSC and WTF headers include PrintStream.h - and no, it doesn't change often.
Tim Horton
Comment 11 2013-08-12 12:12:31 PDT
Antonio Gomes
Comment 12 2013-08-12 12:41:50 PDT
I remember seeing objections for such kind of print methods addition to these classes.
Antonio Gomes
Comment 13 2013-08-12 12:43:22 PDT
(In reply to comment #12) > I remember seeing objections for such kind of print methods addition to these classes. https://bugs.webkit.org/show_bug.cgi?id=79176 - [Bug 79176] Add dump method for WebCore::Int{Point, Rect, Size}
Tim Horton
Comment 14 2013-08-12 12:55:43 PDT
(In reply to comment #13) > (In reply to comment #12) > > I remember seeing objections for such kind of print methods addition to these classes. > > https://bugs.webkit.org/show_bug.cgi?id=79176 - [Bug 79176] Add dump method for WebCore::Int{Point, Rect, Size} Yeah, they've come back up again on #webkit too.
Note You need to log in before you can comment on or make changes to this bug.