This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Addresses LWG3881.
ClosedPublic

Authored by Mordante on Feb 17 2023, 9:56 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG24c251d94daa: [libc++][format] Addresses LWG3881.
Summary
LWG3881 Incorrect formatting of container adapters backed by std::string

Diff Detail

Event Timeline

Mordante created this revision.Feb 17 2023, 9:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 9:56 AM
Mordante requested review of this revision.Feb 17 2023, 9:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 9:56 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.
libcxx/include/__format/container_adaptor.h
42

Any reason why you're using ref_view instead of all_t? Those are not always equivalent -- is the difference not relevant?

Mordante added inline comments.Mar 3 2023, 12:43 PM
libcxx/include/__format/container_adaptor.h
42

Mainly since that is what the wording in the LWG issue is. Victor's original proposed wording did use views::all_t.
This is the rationale in the issue to make the change:

[2023-02-10 Tim provides updated wording]

The container elements may not be const-formattable so we cannot use the const formatter unconditionally. Also the current wording is broken because an adaptor is not range and we cannot use fmt-maybe-const on the adaptor — only the underlying container.
tcanens added a subscriber: tcanens.Mar 3 2023, 8:33 PM
tcanens added inline comments.
libcxx/include/__format/container_adaptor.h
42

This is for passing a slightly disguised container into a function, not for view composition. It does not need the sometimes-by-reference-sometimes-by-copy-sometimes-ill-formed behavior of all_t.

Mordante added inline comments.Mar 4 2023, 5:04 AM
libcxx/include/__format/container_adaptor.h
42

Thanks for the information.

ldionne accepted this revision.Mar 7 2023, 9:15 AM
ldionne added inline comments.
libcxx/include/__format/container_adaptor.h
42

Ah thanks, I think I had missed that the original resolution was superseded.

libcxx/test/std/containers/container.adaptors/container.adaptors.format/format.functions.tests.h
254
This revision is now accepted and ready to land.Mar 7 2023, 9:15 AM
Mordante marked 3 inline comments as done.Mar 7 2023, 10:06 AM
This revision was automatically updated to reflect the committed changes.