Page MenuHomePhabricator

[libc++] Add missing __format__ attributes
ClosedPublic

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

Details

Reviewers
uabelho
Mordante
Group Reviewers
Restricted Project
Commits
rG7dc9a03cfd78: [libc++] Add missing __format__ attributes
Summary

-Wformat-nonliteral was turned on in https://reviews.llvm.org/D112927,
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.Mon, Nov 15, 2:55 AM
uabelho requested review of this revision.Mon, Nov 15, 2:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 15, 2:55 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Tue, Nov 16, 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 🤦🏼‍♂️.

libcxx/include/__bsd_locale_fallbacks.h
112

I believe we should instead move those to _LIBCPP_FORMAT_PRINTF. The usage is documented here: https://clang.llvm.org/docs/AttributeReference.html#format.

This revision now requires changes to proceed.Tue, Nov 16, 9:48 AM
uabelho added inline comments.Tue, Nov 16, 9:53 AM
libcxx/include/__bsd_locale_fallbacks.h
112

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.Tue, Nov 16, 12:22 PM
ldionne edited reviewers, added: uabelho; removed: ldionne.
ldionne added inline comments.
libcxx/include/__bsd_locale_fallbacks.h
112

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

ldionne updated this revision to Diff 387734.Tue, Nov 16, 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.Tue, Nov 16, 1:27 PM
Mordante added a subscriber: Mordante.

LGTM modulo one remark.

libcxx/include/__config
1379–1382

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.Tue, Nov 16, 10:24 PM
libcxx/include/__bsd_locale_fallbacks.h
112

Great! Thank you!

ldionne updated this revision to Diff 389869.Thu, Nov 25, 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.Thu, Nov 25, 1:06 PM

Will ship once CI passes.

libcxx/include/__config
1379–1382

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.Thu, Nov 25, 1:06 PM
This revision was automatically updated to reflect the committed changes.