[libcxx-commits] [PATCH] D142285: [libc++] Don't include <version> everywhere

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Even after discussing this during live review, I am not sure this is worth doing. I understand that the goal is to make us more strict w.r.t. to the standard and remove a potential portability trap for users, and I think that's an interesting goal to pursue. I'm usually pretty supportive of those changes. However, in this case, I am just not convinced that the benefit outweighs the cost of doing this. Concretely, I don't think this bites any user (please LMK if I'm wrong here), and the cost for us is to have additional duplication in our headers.



================
Comment at: libcxx/include/atomic:1524
 
-#if defined(__cpp_lib_atomic_is_always_lock_free)
+#if _LIBCPP_STD_VER >= 17
   static _LIBCPP_CONSTEXPR bool is_always_lock_free = __libcpp_is_always_lock_free<__cxx_atomic_impl<_Tp> >::__value;
----------------
Can we get a separate patch for *just this change*? I think I remember there was a pretty tricky bug around this very definition, and I'd like to evaluate the change on its own. To be clear, I like using `_LIBCPP_STD_VER` more than the macro, but I'm not sure that'll work here, which is what I want to think about in that other review.

Edit: I just checked with D133377 and I think this is correct, but please extract into a separate review since this is effectively a NFC.


================
Comment at: libcxx/include/scoped_allocator:734
+// This part of the file is generated by generate_feature_test_macro_components.py. Do not edit it manually.
+// ftm_list_begin
+
----------------
This is not sufficiently visible IMO, the tag should be something like `BEGIN-FEATURE-TEST-MACROS` or something like that.


================
Comment at: libcxx/include/version:148
 __cpp_lib_ranges_starts_ends_with                       202106L <algorithm>
-__cpp_lib_ranges_to_container                           202202L <deque> <forward_list> <list>
-                                                                <map> <priority_queue> <queue>
-                                                                <set> <stack> <string>
-                                                                <unordered_map> <unordered_set> <vector>
+__cpp_lib_ranges_to_container                           202202L <ranges>
 __cpp_lib_ranges_zip                                    202110L <ranges> <tuple> <utility>
----------------
Also to be moved to a different review, please!


================
Comment at: libcxx/test/libcxx/transitive_includes/cxx11.csv:122
 cinttypes cstdint
+cmath cstddef
+cmath limits
----------------
Can you please figure out why this is detected now and wasn't before? This change should not be needed.


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:564
     "unimplemented": True,
   }, {
     "name": "__cpp_lib_ranges_starts_ends_with",
----------------
If we ever move forward with this patch, I think we'd also want some sort of test to check that including e.g. `<vector>` does not define FTMs from e.g. `<atomic>`. This is hard to achieve, though -- we'd need a full granularization and public headers would only include the private headers they need to provide the standard API, and nothing else. And inside the library, implementation-detail headers would never include any public API header. That may be interesting to pursue, but we're not quite there yet.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142285/new/

https://reviews.llvm.org/D142285