Bug 75605 – WTF logging functions should call vprintf_stderr_common only once per line

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Mark Rowe (bdash)]

Currently several of the WTF logging functions make multiple calls to vprintf_stderr_common to output a single line of text. This results in strangely formatted output if vprintf_stderr_common is retargeted to an output device such as syslog that is message-oriented rather than stream-oriented.

[Mark Rowe (bdash)]

Created attachment 121227 [details]
Patch v1

[WebKit Review Bot]

Attachment 121227 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/Assertions.cpp:130:  vprintf_stderr_with_prefix is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/Assertions.cpp:139:  vprintf_stderr_with_trailing_line is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 121227 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=121227&action=review

> Source/JavaScriptCore/wtf/Assertions.cpp:134
> +    strcat(formatWithPrefix.get(), format);

This is effectively strlen()ing prefix again. You could have done this a little more efficiently.

[Early Warning System Bot]

Comment on attachment 121227 [details]
Patch v1

Attachment 121227 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11128164

[Mark Rowe (bdash)]

Landed in r104124.

[Csaba Osztrogonác]

Comment on attachment 121227 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=121227&action=review

> Source/JavaScriptCore/wtf/Assertions.cpp:128
> +#if COMPILER(GCC)
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wformat-nonliteral"
> +#endif

This change is absolutely incorrect, because push introduced in gcc 4.6.
We can't expect that everyone use this gcc.

We should use this pragma inside a similar guard:
#if defined(__GNUC__) && (__GNUC__ * 100 + __GNUC_MINOR__ >= 406)
...
#endif

[Csaba Osztrogonác]

GCC_VERSION_AT_LEAST(4, 6, 0) is better.

[Mark Rowe (bdash)]

That would probably prevent it from being picked up by clang.

[Mark Rowe (bdash)]

I'd guess that something like the following would do the trick:

#if COMPILER(CLANG) || (COMPILER(GCC) && GCC_VERSION_AT_LEAST(4, 6, 0))

I have no way to test it with GCC though.

[Zoltan Herczeg]

(In reply to comment #9)
> I'd guess that something like the following would do the trick:
> 
> #if COMPILER(CLANG) || (COMPILER(GCC) && GCC_VERSION_AT_LEAST(4, 6, 0))
> 
> I have no way to test it with GCC though.

I have already landed http://trac.webkit.org/changeset/104134. Shall I add the CLANG test?

[Mark Rowe (bdash)]

Given that your change broke all of the Mac builds, please do.

[Zoltan Herczeg]

(In reply to comment #11)
> Given that your change broke all of the Mac builds, please do.

Sorry for that. Buildfix landed.