This is the first step at implementing <format>. It adds the <format> header
and implements the format_error. class.
Implemnts parts of:
-P0645 Text Formatting
Paths
| Differential D92214
[libc++] Implement format_error. ClosedPublic Authored by Mordante on Nov 27 2020, 1:10 AM.
Details
Summary This is the first step at implementing <format>. It adds the <format> header Implemnts parts of:
Diff Detail
Unit TestsFailed Event TimelineMordante marked an inline comment as done. Comment ActionsThanks for the review! It seems a build bot fails. I'll investigate this further.
Mordante marked an inline comment as done. Comment ActionsProperly guard the code in format.cpp to only be used in C++20 mode. Comment Actions To fix Apple back-deployment build, you'll need to mark your test with sth like: // This test requires the dylib support introduced in D92214. // XFAIL: with_system_cxx_lib=macosx10.15 // XFAIL: with_system_cxx_lib=macosx10.14 // XFAIL: with_system_cxx_lib=macosx10.13 // XFAIL: with_system_cxx_lib=macosx10.12 // XFAIL: with_system_cxx_lib=macosx10.11 // XFAIL: with_system_cxx_lib=macosx10.10 // XFAIL: with_system_cxx_lib=macosx10.9 because this build uses a released dylibs for the given version of macOS (here 10.9) and does not use whatever changes you put in src/. Comment Actions
Thanks for the hint! Comment Actions Looks okay except for the synopsis nitpick, however as you say the Apple ABI list needs to be updated. If you rebase on top of master and re-submit, the failing jobs should tell you what symbols you're missing (in the logs). You could add them to the Apple ABI lists (please make sure to sort :-), and the CI should pass.
This revision now requires changes to proceed.Dec 1 2020, 2:28 PM Comment Actions Thanks for the review.
@curdeius already committed the Buildkite ABI list improvements. So I copied the Apple ABI list artifact of the failed build :-) A lot easier than looking in the logs ;-)
curdeius added inline comments.
This revision now requires changes to proceed.Dec 2 2020, 12:04 PM Mordante marked an inline comment as done. Comment ActionsImplement all new header tests as suggested by @curdeius. Mordante added a child revision: D93166: [libc++][format] Add basic_format_parse_context..Dec 12 2020, 11:04 AM ldionne added inline comments.
This revision now requires changes to proceed.Jan 19 2021, 8:08 AM Mordante added inline comments. Mordante marked 2 inline comments as done. Comment Actions
Comment Actions Thanks! The CI has been a bit grumpy the last days, but hopefully it should be back to normal now. This revision is now accepted and ready to land.Jan 28 2021, 8:19 AM Closed by commit rG081c1db02dd2: [libc++] Implement format_error. (authored by Mordante). · Explain WhyJan 28 2021, 9:03 AM This revision was automatically updated to reflect the committed changes. Comment Actions Hello, this again breaks building with -std=c++17: the cpp file has the definition but not the declaration in that case. Should libc++ sources be built with -std=c++20? Or should the cpp file also check _LIBCPP_STD_VER? Comment Actions ... but it looks like CXX_STANDARD_REQUIRED should now become YES after this change here, right? Comment Actions
I had this in my original version, but I removed it after review this review comment
That doesn't really answer the question since that made C++20 preferred but not mandatory. That was the intention. @ldionne Can you clarify the status of libc++'s C++20 requirements. If optional should I re-add the version check macro's in the .cpp file? Comment Actions
Ugh. That's tough. First, we can't always build as C++17 because then the dylib can't provide the features it needs to support c++20 (unless we jump through some hoops to compile with C++17 but enable stuff like char8_t, which is a no-no). Comment Actions
Indeed, not using C++20 to compile the library would prevent us from implementing any C++20 features, char8_t support is one example, format_error is another. Just my 2 cents. Anyway, guarding C++20 features in src/ so as to allow the build and use in C++17 would probably be the way to go for the moment. Comment Actions I agree it would be nice to allow C++17 and C++20 for a limited time. It would be a bit of a maintenance burden, but I feel it benefits our users. I think we can state users are allowed to use C++17, but it's not officially supported. We could make a Cmake switch to opt-in to this behavior, but I feel that would be overkill. I think once C++20 becomes mandatory we should remove the #if _LIBCPP_STD_VER > 17 from the files in src. Then the extra burden is only there for a limited time. I'll make a patch to remove the 2 occurrences of #if _LIBCPP_STD_VER > 11 from src. Comment Actions
Yes, I agree.
No, that's actually what we do all the time. We build the dylib in C++20, but users are free to use the headers in C++03, C++11, C++14, C++17 or C++20 mode. @Mordante Feel free to add back the check. However, in the short/mid term, I want the default way to build libc++ to switch to the bootstrap build (i.e. the Runtimes build). Once that happens, we will be guaranteed that we always build the dylib with basically Clang trunk (or a recent compiler anyway). It won't make sense anymore to allow anything but the latest standard then. It is indeed a maintenance burden to support more than that, and a burden that (I claim) doesn't provide much benefit. If you're compiling the C++ Standard Library from scratch, I think you have the time and dedication to keep your compiler up to date. Comment Actions
Yes, right. A brain fart from my side. Comment Actions
I'll commit a fix.
Sounds good to me.
Revision Contents
Diff 308009 libcxx/docs/Cxx2aStatusPaperStatus.csv
libcxx/docs/FeatureTestMacroTable.rst
libcxx/include/CMakeLists.txt
libcxx/include/format
libcxx/include/module.modulemap
libcxx/include/version
libcxx/lib/abi/x86_64-unknown-linux-gnu.v1.abilist
libcxx/src/CMakeLists.txt
libcxx/src/format.cpp
libcxx/test/libcxx/double_include.sh.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/format.version.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/version.version.pass.cpp
libcxx/test/std/utilities/format/format.error/format.error.pass.cpp
libcxx/test/std/utilities/format/version.compile.pass.cpp
libcxx/utils/generate_feature_test_macro_components.py
|
The synopsis should only contain the parts that we implement. Or at least it's always been my impression.