[libcxx-commits] [PATCH] D144322: [libc++][Modules] Make top level modules for all C++ headers with OS/clang versions

[Mark de Wever via Phabricator via libcxx-commits]

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

I have not reviewed the entire patch.
Please update the commit message to describe why these changes are needed and what the benefit is. It looks some parts are wrong; removing headers for forward declarations, while the contents of the header are used, is wrong. It might compile due to transitive includes, but we really should include the proper headers.



================
Comment at: libcxx/include/__format/buffer.h:32
 #include <cstddef>
-#include <string_view>
 #include <type_traits>
----------------
This is wrong, the compiler needs to know the details of the `string_view`.


================
Comment at: libcxx/include/__format/buffer.h:105
     if (__n <= __capacity_) {
       _VSTD::copy_n(__str.data(), __n, _VSTD::addressof(__ptr_[__size_]));
       __size_ += __n;
----------------
Here we call member functions of `string_view`.


================
Comment at: libcxx/include/__format/format_arg.h:24-25
 #include <__variant/monostate.h>
-#include <string>
-#include <string_view>
 
----------------
Why are these two replaced with the `__fwd` version? The `string_view` seems wrong, since it's used as a member variable.


================
Comment at: libcxx/include/__format/format_arg.h:200
     const _CharT* __const_char_type_ptr_;
     basic_string_view<_CharT> __string_view_;
     const void* __ptr_;
----------------
Here the string_view is used a member variable.


================
Comment at: libcxx/include/module.modulemap.in:1
-// define the module for __config outside of the top level 'std' module
-// since __config may be included from C headers which may create an
----------------
Changing this file will cause merge conflicts with several in-flight patches. That is not a blocker per se, however I really want the commit message explain what the benefit of this change is.


================
Comment at: libcxx/include/module.modulemap.in:238-239
+      module min                             { export std___algorithm_min }
+      module min_element                     { export std___algorithm_min_element }
+      module min_max_result                  { export std_algorithm.__algorithm.min_max_result }
       module minmax                          {
----------------
I see here two different ways used for the export. What is the difference between them and when use the first and when the second.


================
Comment at: libcxx/test/support/test_macros.h:144
 #if defined(__cpp_lib_is_constant_evaluated) && __cpp_lib_is_constant_evaluated >= 201811L
+# include <__type_traits/is_constant_evaluated.h>
 # define TEST_IS_CONSTANT_EVALUATED std::is_constant_evaluated()
----------------
Please remove this change. This only works for libc++, not for other Standard libraries. When this macro is used, the test should include `type_traits` itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322