Bug 207478 – a lot gcc warnings because of %{public}s format specifier

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

[Víctor M. Jáquez L.]

While compiling with GCC a lot of big warnings are shown because those macros boil down to vfprintf() in Linux, and that {public} specifier is invalid. It is only for os_log [1]


1. https://developer.apple.com/documentation/os/logging?language=objc

[Víctor M. Jáquez L.]

Created attachment 390262 [details]
Patch

[Darin Adler]

Comment on attachment 390262 [details]
Patch

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

> Source/WTF/wtf/Assertions.h:487
> +#define LOGDSTR "s"

In WebKit coding style we normally prefer words over clumps of letters. I don’t know that LOGDSTR is a good choice for the project. Is this referring to something named "logd"? Maybe PUBLIC_LOG_STRING would be a good name? Also, why not put the "%" into the macro?

[Keith Rollin]

(In reply to Darin Adler from comment #2)
> Also, why not put the "%" into the macro?

I think that leaving the "%" out of the macro follows standard macros like PRId64.

Any naming convention should take into account other types of format specifications that might be used. For instance:

PUBLIC_LOG_STRING: {public}s
PRIVATE_LOG_STRING: {private}s
FUTUREOPTION_LOG_STRING: {future-option}s

PUBLIC_LOG_OBJCOBJECT: {public}@
PRIVATE_LOG_OBJCOBJECT: {private}@
FUTUREOPTION_LOG_OBJCOBJECT: {future-option}@

So while I like LOGDSTR for its brevity, it doesn't scale.

[Darin Adler]

(In reply to Keith Rollin from comment #3)
> (In reply to Darin Adler from comment #2)
> > Also, why not put the "%" into the macro?
> 
> I think that leaving the "%" out of the macro follows standard macros like
> PRId64.

It does. That alone doesn’t seem like reason to follow suit, though.

[Keith Rollin]

(In reply to Darin Adler from comment #4)
> (In reply to Keith Rollin from comment #3)
> > (In reply to Darin Adler from comment #2)
> > > Also, why not put the "%" into the macro?
> > 
> > I think that leaving the "%" out of the macro follows standard macros like
> > PRId64.
> 
> It does. That alone doesn’t seem like reason to follow suit, though.

What's the reason for not following suit?

Personally, I'd like to be consistent with the standard. That way I don't have to remember "OK, add the % for the standard macros, but leave it out for WebKit macros. Or is it the other way around...".

[Víctor M. Jáquez L.]

(In reply to Keith Rollin from comment #5)
> (In reply to Darin Adler from comment #4)
> > (In reply to Keith Rollin from comment #3)
> > > (In reply to Darin Adler from comment #2)
> > > > Also, why not put the "%" into the macro?
> > > 
> > > I think that leaving the "%" out of the macro follows standard macros like
> > > PRId64.
> > 
> > It does. That alone doesn’t seem like reason to follow suit, though.
> 
> What's the reason for not following suit?
> 
> Personally, I'd like to be consistent with the standard. That way I don't
> have to remember "OK, add the % for the standard macros, but leave it out
> for WebKit macros. Or is it the other way around...".

I do agree. It is better, IMO, to be consistent with other formatting macros.

[Víctor M. Jáquez L.]

Created attachment 390505 [details]
Patch

[Víctor M. Jáquez L.]

(In reply to Keith Rollin from comment #3)
> (In reply to Darin Adler from comment #2)
> > Also, why not put the "%" into the macro?
> 
> I think that leaving the "%" out of the macro follows standard macros like
> PRId64.
> 
> Any naming convention should take into account other types of format
> specifications that might be used. For instance:
> 
> PUBLIC_LOG_STRING: {public}s

Done. Now this symbol is used.

> PRIVATE_LOG_STRING: {private}s
> FUTUREOPTION_LOG_STRING: {future-option}s
> 
> PUBLIC_LOG_OBJCOBJECT: {public}@
> PRIVATE_LOG_OBJCOBJECT: {private}@
> FUTUREOPTION_LOG_OBJCOBJECT: {future-option}@
> 
> So while I like LOGDSTR for its brevity, it doesn't scale.

[Darin Adler]

(In reply to Keith Rollin from comment #5)
> What's the reason for not following suit?

To me PUBLIC_LOG_STRING doesn’t seem much like PRId64. I agree that if you thought of them as similar and closely related then we’d want them to match.

> Personally, I'd like to be consistent with the standard. That way I don't
> have to remember "OK, add the % for the standard macros, but leave it out
> for WebKit macros. Or is it the other way around...".

OK, tastes differ, and I certainly won’t insist.

[Víctor M. Jáquez L.]

May I have r+ & q+ ? 

O:)

[WebKit Commit Bot]

Comment on attachment 390505 [details]
Patch

Clearing flags on attachment: 390505

Committed r256745: <https://trac.webkit.org/changeset/256745>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59517129>