RESOLVED WONTFIX 59830
Add WTFPrintf()
https://bugs.webkit.org/show_bug.cgi?id=59830
Summary Add WTFPrintf()
Jeff Miller
Reported 2011-04-29 14:10:16 PDT
Add WTFPrintf()
Attachments
Add WTFPrintf() (3.12 KB, patch)
2011-04-29 14:14 PDT, Jeff Miller
no flags
Update patch to include PRINTF macro, based on feedback from Dan. (3.61 KB, patch)
2011-04-29 15:45 PDT, Jeff Miller
ap: review-
Jeff Miller
Comment 1 2011-04-29 14:14:38 PDT
Created attachment 91737 [details] Add WTFPrintf()
Jeff Miller
Comment 2 2011-04-29 15:45:12 PDT
Created attachment 91756 [details] Update patch to include PRINTF macro, based on feedback from Dan.
Alexey Proskuryakov
Comment 3 2011-04-29 17:42:57 PDT
How is this better than a plain fprintf(stderr, ...)?
Jeff Miller
Comment 4 2011-04-29 17:54:14 PDT
(In reply to comment #3) > How is this better than a plain fprintf(stderr, ...)? That won't work on all platforms, which is why we have WTFLog() in the first place. See vprintf_stderr_common() in Assertions.cpp, which is where everything bottlenecks through.
Alexey Proskuryakov
Comment 5 2011-04-29 18:07:44 PDT
Thank you, I now see what the technical difference is. What is the use case for WTFPrint? When do we want to print from the current process to error log unconditionally, and without passing the message to developer console? I don't think that features like this can be added without at least one use case documented in the bug.
Jeff Miller
Comment 6 2011-04-29 18:14:01 PDT
(In reply to comment #5) > Thank you, I now see what the technical difference is. > > What is the use case for WTFPrint? When do we want to print from the current process to error log unconditionally, and without passing the message to developer console? I don't think that features like this can be added without at least one use case documented in the bug. The use case is the classic "printf debugging" idiom, where you're trying to debug something that's difficult to reproduce by stepping in the debugger because of timing issues, for example. Today, I have to resort to doing something like: WTFLogChannel LogAlways = { WTFLogChannelOn, 0, 0}; LOG(Always, "this is a test"); which is a pain. I don't forsee any instances where you would actually check in code that uses PRINTF().
Alexey Proskuryakov
Comment 7 2011-04-29 20:03:37 PDT
Thanks Jeff, that makes sense. Since I'm usually debugging on Mac, plain printf works great for me, but I can see how this can be difficult on Windows, even with /console switch. And LOG_ERROR output is probably not very suitable due to its ERROR: prefix. > I don't forsee any instances where you would actually check in code that uses PRINTF(). I'm wondering if permanently adding an Always channel would communicate this better. The reason I'm asking is that the choice of macros in Assertions.h is already fairly confusing (do I use LOG or LOG_ASSERT?..) Not sure if the idea with LOG(Always) is better than PRINTF tbhough
Jeff Miller
Comment 8 2011-04-30 14:15:09 PDT
(In reply to comment #7) > Thanks Jeff, that makes sense. > > Since I'm usually debugging on Mac, plain printf works great for me, but I can see how this can be difficult on Windows, even with /console switch. And LOG_ERROR output is probably not very suitable due to its ERROR: prefix. > > > I don't forsee any instances where you would actually check in code that uses PRINTF(). > > I'm wondering if permanently adding an Always channel would communicate this better. The reason I'm asking is that the choice of macros in Assertions.h is already fairly confusing (do I use LOG or LOG_ASSERT?..) Not sure if the idea with LOG(Always) is better than PRINTF tbhough Personally, I vote for PRINTF(), it's straightforward and most people understand what it means.
Alexey Proskuryakov
Comment 9 2011-04-30 15:33:10 PDT
Comment on attachment 91756 [details] Update patch to include PRINTF macro, based on feedback from Dan. View in context: https://bugs.webkit.org/attachment.cgi?id=91756&action=review OK, I'm fine with adding PRINTF. I think that I have too many comments to justify another quick review round though. > Source/JavaScriptCore/wtf/Assertions.cpp:299 > + if (format[strlen(format) - 1] != '\n') What if format is zero length? > Source/JavaScriptCore/wtf/Assertions.h:363 > +#define PRINTF() ((void)0) Perhaps #define PRINTF printf ? > Source/JavaScriptCore/wtf/Assertions.h:365 > +#define PRINTF(arg...) ((void)0) Ditto. > Source/JavaScriptCore/wtf/Assertions.h:367 > +#elif LOG_DISABLED > +#define PRINTF(...) ((void)0) I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too.
Jeff Miller
Comment 10 2011-04-30 15:54:44 PDT
(In reply to comment #9) > (From update of attachment 91756 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91756&action=review > > OK, I'm fine with adding PRINTF. I think that I have too many comments to justify another quick review round though. > > > Source/JavaScriptCore/wtf/Assertions.cpp:299 > > + if (format[strlen(format) - 1] != '\n') > > What if format is zero length? I'll fix this bug that was in the existing code. > > > Source/JavaScriptCore/wtf/Assertions.h:363 > > +#define PRINTF() ((void)0) > > Perhaps #define PRINTF printf ? See my comments below. > > > Source/JavaScriptCore/wtf/Assertions.h:365 > > +#define PRINTF(arg...) ((void)0) > > Ditto. > > > Source/JavaScriptCore/wtf/Assertions.h:367 > > +#elif LOG_DISABLED > > +#define PRINTF(...) ((void)0) > > I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too. Dan was adamant that it should be turned off when logging is disabled. I originally had it enabled all the time. I don't have a strong preference either way, if you feel strongly you should try to convince Dan.
Alexey Proskuryakov
Comment 11 2011-04-30 15:57:44 PDT
We should try to convince Dan :-)
Jeff Miller
Comment 12 2011-05-02 10:46:57 PDT
(In reply to comment #9) > (From update of attachment 91756 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91756&action=review > > > Source/JavaScriptCore/wtf/Assertions.h:363 > > +#define PRINTF() ((void)0) > > Perhaps #define PRINTF printf ? > > > Source/JavaScriptCore/wtf/Assertions.h:365 > > +#define PRINTF(arg...) ((void)0) > > Ditto. > I'd be hesitant to do this, just because we don't implement LOG() and the other macros on these platforms at all. Seems like it would be better to match the existing idiom.
mitz
Comment 13 2011-05-02 10:50:27 PDT
(In reply to comment #9) > I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too. It’s generally bad form for production code, especially a framework with arbitrary clients, to do any writing to stdout/stderr. Who is supposed to read this?
Jeff Miller
Comment 14 2011-05-02 10:58:49 PDT
(In reply to comment #13) > (In reply to comment #9) > > I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too. > > It’s generally bad form for production code, especially a framework with arbitrary clients, to do any writing to stdout/stderr. Who is supposed to read this? I would assume that the engineer writing the code will want to read this when reproducing a bug that doesn't happen in debug code, for example. Of course, you could also set LOG_DISABLED to 0 temporarily in production in this case, or just call WTFPrintf() directly, since it (as well as the other logging routines) are always implemented, regardless of whether LOG_DISABLED is set or not.
mitz
Comment 15 2011-05-02 11:04:03 PDT
(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #9) > > > I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too. > > > > It’s generally bad form for production code, especially a framework with arbitrary clients, to do any writing to stdout/stderr. Who is supposed to read this? > > I would assume that the engineer writing the code will want to read this when reproducing a bug that doesn't happen in debug code, for example. Of course, you could also set LOG_DISABLED to 0 temporarily in production in this case, or just call WTFPrintf() directly, since it (as well as the other logging routines) are always implemented, regardless of whether LOG_DISABLED is set or not. That would be one way of doing it. I mostly want to minimize the risk of spurious stderr output making it into production builds. Perhaps making the distinction Debug/Release vs. Production instead of Debug vs. Production/Release is the right way to do this, but traditionally I think we’ve kept Release as similar as possible to Production.
Jeff Miller
Comment 16 2011-05-02 11:14:10 PDT
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > (In reply to comment #9) > > > > I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too. > > > > > > It’s generally bad form for production code, especially a framework with arbitrary clients, to do any writing to stdout/stderr. Who is supposed to read this? > > > > I would assume that the engineer writing the code will want to read this when reproducing a bug that doesn't happen in debug code, for example. Of course, you could also set LOG_DISABLED to 0 temporarily in production in this case, or just call WTFPrintf() directly, since it (as well as the other logging routines) are always implemented, regardless of whether LOG_DISABLED is set or not. > > That would be one way of doing it. I mostly want to minimize the risk of spurious stderr output making it into production builds. Perhaps making the distinction Debug/Release vs. Production instead of Debug vs. Production/Release is the right way to do this, but traditionally I think we’ve kept Release as similar as possible to Production. Yeah, I agree with minimizing risk. I personally think it's OK to turn off PRINTF() when LOG_DISABLE is on, since there are straight-forward ways around this if you really need to do a PRINTF() in a Production/Release build (which should be rare anyway).
Jeff Miller
Comment 17 2011-05-02 11:22:34 PDT
(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 91756 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=91756&action=review > > > > > Source/JavaScriptCore/wtf/Assertions.cpp:299 > > > + if (format[strlen(format) - 1] != '\n') > > > > What if format is zero length? > I wrote up a separate bug (https://bugs.webkit.org/show_bug.cgi?id=59949) to fix the existing code in WTFLog() and WTFLogVerbose().
Alexey Proskuryakov
Comment 18 2011-05-02 11:23:29 PDT
Rebuilding with a new LOG_DISABLED value would be huge and unnecessary pain. And again, having a LOG-related setting control PRINTF would be hugely confusing in my opinion. I don't think there is anything special to protect from here. Poor review can let code with printf get landed, and it's the same with PRINTF.
Alexey Proskuryakov
Comment 19 2011-05-02 11:26:00 PDT
Is there a way to resolve the practical problem by making printf() work as required for debugging instead? I think that there must be such a way.
Jeff Miller
Comment 20 2011-05-02 11:31:55 PDT
I think it's best to just table this for now, since there's a lot of disagreement about this.
Note You need to log in before you can comment on or make changes to this bug.