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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__iterator/advance.h:73
 // [range.iter.op.advance]
 struct __advance_fn final : __function_like {
 private:
----------------
Btw, should there be a `private` here? `__next_fn` and `__prev_fn` both use private inheritance here, not public inheritance. At least we should be consistent.
(obsoleted by D106567)


================
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"
----------------
Mordante wrote:
> cjdb wrote:
> > 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.
> > There should be no need to `export __function_like` here.
> I agree, but clang disagrees. I had the same issue in my format series, before we made these headers private.
> I know no other solution than what @cjdb did here.
I've looked into it a bit, and uploaded D106567 with a simpler fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105078