[libcxx-commits] [PATCH] D105078: [libcxx][modularisation] properly modularises advance, next, and prev

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 2 inline comments as done.
cjdb added inline comments.


================
Comment at: libcxx/include/module.modulemap:481
       module access                { private header "__iterator/access.h" }
-      module advance               { private header "__iterator/advance.h" }
+      module advance               {
+        private header "__iterator/advance.h"
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > I don't like the formatting here. But whatever. 
> There should be no need to `export __function_like` here. Then it'll all fit on one line, consistent with all the other lines, so there won't be a formatting problem anymore.
> I don't like the formatting here. But whatever. 

Yeah, I didn't notice till after I threw it up on Phab. Will fix before merging.

> There should be no need to `export __function_like` here. Then it'll all fit on one line, consistent with all the other lines, so there won't be a formatting problem anymore.

There is a need to export `__function_like`. I've gone into this in other patches before, so I'm not going to repeat myself here.


================
Comment at: libcxx/include/module.modulemap:786
   module __errc              { private header "__errc"              export * }
   module __function_like     { private header "__function_like.h"   export * }
   module __hash_table        { header "__hash_table"        export * }
----------------
Quuxplusone wrote:
> Probably this `private` is the original problem. What happens if you remove it?
> (Compare to `__hash_table`, `__nullptr`, etc.)
Nope. Nothing gets fixed because this isn't the problem.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/special_function.compile.pass.cpp:17
-// XFAIL: clang-12 && modules-build
-// XFAIL: clang-13 && modules-build
-
----------------
zoecarver wrote:
> Don't we still support clang-13 or am I missing something? Does this patch work around the bug? 
This patch isn't a workaround: it's the correct fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105078