This is an archive of the discontinued LLVM Phabricator instance.

[libc++][NFC] Move format_error to its own header.
ClosedPublic

Authored by Mordante on May 2 2021, 10:08 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG16342e39947b: [libc++][NFC] Move format_error to its own header.

Diff Detail

Event Timeline

Mordante created this revision.May 2 2021, 10:08 AM
Mordante requested review of this revision.May 2 2021, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2021, 10:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I don't see anything stylistically wrong with this, but I'd like to see a rationale for why it's a good idea. What does having-this-in-a-separate-header buy us, physically?

libcxx/include/__format/format_error.h
10–11

_LIBCPP___FORMAT_FORMAT_ERROR_H

Thanks for the review!

I don't see anything stylistically wrong with this, but I'd like to see a rationale for why it's a good idea. What does having-this-in-a-separate-header buy us, physically?

I personally like small headers/files. I find them easier to navigate than large files. Since it seems libc++ is moving in that direction I'm happy to oblige. In this case there's another reason. Locally I've more std::format patches. Managing patch series where parts are stored in separate files makes it easier when rebasing the series. (These patches will be up for review in a later stage, for new I'm still improving them.)

libcxx/include/__format/format_error.h
10–11

Will fix.

Mordante updated this revision to Diff 343471.May 6 2021, 12:03 PM

Addresses review comments.

Mordante marked an inline comment as done.May 6 2021, 12:04 PM

friendly ping.

zoecarver added inline comments.
libcxx/include/format
81

I don't feel too strongly, but I think it might be better to move basic_format_parse_context into its own header, and format_error along with it.

Mordante updated this revision to Diff 347201.May 22 2021, 6:35 AM

Rebased to fix applying D93593.

ldionne accepted this revision.May 25 2021, 1:23 PM
This revision is now accepted and ready to land.May 25 2021, 1:23 PM
This revision was automatically updated to reflect the committed changes.