Page MenuHomePhabricator

[libc++] Add missing __format__ attributes

Authored by ldionne on Nov 15 2021, 2:55 AM.


Group Reviewers
Restricted Project
rG7dc9a03cfd78: [libc++] Add missing __format__ attributes

-Wformat-nonliteral was turned on in,
however we forgot to apply some format attributes in Linux specific
code paths, which led to warnings when building on Linux. This patch
addresses that oversight.

Diff Detail

Event Timeline

uabelho created this revision.Nov 15 2021, 2:55 AM
uabelho requested review of this revision.Nov 15 2021, 2:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2021, 2:55 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Nov 16 2021, 9:48 AM

Thanks for looking into this! I missed those because I only built the library on Darwin, and CI passed since those are only warnings 🤦🏼‍♂️.


I believe we should instead move those to _LIBCPP_FORMAT_PRINTF. The usage is documented here:

This revision now requires changes to proceed.Nov 16 2021, 9:48 AM
uabelho added inline comments.Nov 16 2021, 9:53 AM

Would you feel like doing it if it should be fixed in some other way that just disabling the warning?

I really don't know this stuff, I just saw one of our downstream bots signalling there were a bunch of new warnings and saw you disabled warnings this way in your original patch so did the same thing here.

ldionne commandeered this revision.Nov 16 2021, 12:22 PM
ldionne edited reviewers, added: uabelho; removed: ldionne.
ldionne added inline comments.

Yup, no worries. Thanks for bringing that to our attention!

ldionne updated this revision to Diff 387734.Nov 16 2021, 12:24 PM
ldionne retitled this revision from [libc++] Silence -Wformat-nonliteral warnings in __bsd_locale_fallbacks.h to [libc++] Add missing __format__ attributes.
ldionne edited the summary of this revision. (Show Details)

Update with format attributes instead of pragmas to turn off the warning.

Mordante accepted this revision as: Mordante.Nov 16 2021, 1:27 PM
Mordante added a subscriber: Mordante.

LGTM modulo one remark.

1355 ↗(On Diff #387734)

I like the comment, but let's expand it a bit.

This change looks very familiar. I'm quite sure something similar has been up for review before, but seems it never landed.

uabelho added inline comments.Nov 16 2021, 10:24 PM

Great! Thank you!

ldionne updated this revision to Diff 389869.Nov 25 2021, 1:05 PM
ldionne marked 2 inline comments as done.

Address review comments and avoid adding _LIBCPP_HIDE_FROM_ABI, which breaks the GCC build.
We can address that separately.

ldionne accepted this revision as: Restricted Project.Nov 25 2021, 1:06 PM

Will ship once CI passes.

1355 ↗(On Diff #387734)

Yeah, now that you mention it this does seem to ring a bell for me too. But in any case, it hasn't landed yet so I guess we'll eventually stumble upon the change while clearing the review queue.

This revision is now accepted and ready to land.Nov 25 2021, 1:06 PM
This revision was automatically updated to reflect the committed changes.