This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Improve ABI stability.
ClosedPublic

Authored by Mordante on Dec 21 2021, 10:15 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Commits
rGdfb20d4d19ec: [libc++][format] Improve ABI stability.
Summary

During the review of D115991 @vitaut pointed out the enum shouldn't
depend on whether or not _LIBCPP_HAS_NO_INT128 is defined. The current
implementation lets the enum's ABI depend on this configuration option
without a good cause.

Diff Detail

Event Timeline

Mordante requested review of this revision.Dec 21 2021, 10:15 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2021, 10:15 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Dec 21 2021, 10:31 AM
This revision is now accepted and ready to land.Dec 21 2021, 10:31 AM
Mordante updated this revision to Diff 395730.Dec 21 2021, 11:34 AM

Fix the CI. I had validated that -Wswitch wasn't used on Linux, but it seems
other CI jobs do, leading to an expected error.

ldionne accepted this revision.Dec 21 2021, 12:12 PM

Still LGTM pending CI.

Quuxplusone added inline comments.
libcxx/include/__format/format_arg.h
42–43

Consider replacing this comment with a test in libcxx/test/libcxx/ that does assert(__format::__arg_t::__ptr == 14); and is not skipped when _LIBCPP_HAS_NO_INT128.
Mantra: "If you liked it then you should have put a test on it!" If we're trying to ensure the ABI doesn't break here, let's put a test on that ABI.

108–117

Murphy's Law says that somebody will ignore this comment and add a case somethingelse: on line 106, at which point the comment will be wrong because these cases will fallthrough to that.
I guess they'll fall through only when int128 is unsupported and so we shouldn't be hitting these cases anyway... but still, can you just make both cases explicitly do #else _LIBCPP_UNREACHABLE() #endif instead?

Mordante updated this revision to Diff 395884.Dec 22 2021, 8:51 AM

Addresses review comments and fixes CI.

Mordante marked 2 inline comments as done.Dec 22 2021, 8:54 AM
Mordante added inline comments.
libcxx/include/__format/format_arg.h
42–43

I don't want to remove the comment, but I like your idea of adding a test.

108–117

I was doubting between this solution and the one you suggested. This convinces me the other solution is better.

This revision was automatically updated to reflect the committed changes.
Mordante marked 2 inline comments as done.