> On Feb 27, 2019, at 4:05 PM, Keith Rollin <krol...@apple.com> wrote:
>
>> On Wed, Feb 27, 2019 at 2:36 PM Michael Catanzaro <mcatanz...@igalia.com>
>> wrote:
>> Hi,
>>
>> For the past several years, I've been regularly fixing -Wformat
>> warnings that look like this:
>>
>> ../../Source/WebKit/WebProcess/WebPage/WebPage.cpp:3148:46: warning:
>> format ‘%llu’ expects argument of type ‘long long unsigned
>> int’, but argument 6 has type ‘uint64_t’ {aka ‘long unsigned
>> int’} [-Wformat=]
>> RELEASE_LOG_ERROR(ProcessSuspension, "%p - WebPage
>> (PageID=%llu) - LayerTreeFreezeReason::ProcessSuspended was set when
>> removing LayerTreeFreezeReason::PageTransition; current reasons are %d",
>>
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> this, m_pageID, m_LayerTreeFreezeReasons.toRaw());
>> ~~~~~~~~
>>
>> Problem is that uint64_t is long long unsigned int on Mac, but only
>> long unsigned int on Linux. It can't be printed with %llu, so please
>> use PRIu64 instead. E.g.:
>>
>> LOG("%llu", pageID); // wrong
>> LOG("%" PRIu64, pageID); // correct
>>
>> This is good to keep in mind for any sized integer types, but uint64_t
>> in particular since this is what causes problems for us in practice.
>
>
>
>> On Feb 27, 2019, at 14:47, Ryosuke Niwa <rn...@webkit.org> wrote:
>>
>> We should probably stop using these formatting strings in favor of
>> makeString by making *LOG* functions to use makeString behind the scene.
>
> Formatting strings are pretty much required for the RELEASE_LOG* macros.
> These macros require a string literal for the formatting information, so you
> can’t say something like:
>
> RELEASE_LOG_ERROR(ProcessSuspension, makeString(this, " - WebPage (PageID =
> ", m_pageID, "), - LayerTreeFreezeReason::ProcessSuspended was set when
> removing LayerTreeFreezeReason::PageTransition; current reasons are ",
> m_LayerTreeFreezeReasons.toRaw()));
>
> This would not result in a string literal being passed to RELEASE_LOG, and
> you’d get a compiler error.
>
> You could technically get away with passing your created string along with a
> “%s” format specification:
>
> RELEASE_LOG_ERROR(ProcessSuspension, "%s", makeString(this, " - WebPage
> (PageID = ", m_pageID, "), - LayerTreeFreezeReason::ProcessSuspended was set
> when removing LayerTreeFreezeReason::PageTransition; current reasons are ",
> m_LayerTreeFreezeReasons.toRaw()));
>
> But this is not advised. The underlying Cocoa os_log facility is able to
> greatly compress the logs stored in memory and on disk, as well as get
> corresponding performance benefits, by taking advantage of the fact that the
> formatting string is a literal that can be found in the executable’s binary
> image. When you log with a particular formatting string, that string is not
> included in the log archive, but a reference to it is. In the case that
> Michael gives, a reference to the big formatting string is stored along with
> the pointer, the unsigned long long, and the integer. In the above example, a
> reference to “%s” is stored, along with the fully-formatted string. This
> latter approach takes up a lot more memory.
>
> So go wild with the LOG* macros, but understand the restrictions with the
> RELEASE_LOG* macros.
Seems like a fun project would be to attempt to generate the format string
literal at compile time based on the parameters passed.
- Sam
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
