[libcxx-commits] [PATCH] D106287: [libc++] Add a bunch of missing _LIBCPP_HIDE_FROM_ABI in <ranges>

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/docs/Contributing.rst:34
 - Did you update the relevant files to track implementation status (in ``docs/Status/``)?
+- Did you mark the relevant functions with ``_LIBCPP_HIDE_FROM_ABI``?
 - If you added a header:
----------------
ldionne wrote:
> Mordante wrote:
> > ldionne wrote:
> > > Mordante wrote:
> > > > Shouldn't this be more general?
> > > > `Did you mark all functions and type declarations with the proper visibility macro?`
> > > > Where visibility macro links to https://libcxx.llvm.org/DesignDocs/VisibilityMacros.html
> > > > 
> > > > Maybe, in a separate commit, you can have a look at the VisibilityMacro page. It doesn't list `_LIBCPP_INLINE_VISIBILITY`. I wonder what the difference between _LIBCPP_HIDE_FROM_ABI` and `_LIBCPP_INLINE_VISIBILITY` is. Grepping our codebase I see not many occurrences of `_LIBCPP_HIDE_FROM_ABI`, but a lot of `_LIBCPP_INLINE_VISIBILITY`. Looking at the amount of `_LIBCPP_HIDE_FROM_ABI` used in ranges I start to wonder whether the wrong macro is used at other places.
> > > > 
> > > > 
> > > > Shouldn't this be more general?
> > > 
> > > Hmm, yes, I agree. Will fix.
> > > 
> > > > Maybe, in a separate commit, you can have a look at the VisibilityMacro page. It doesn't list `_LIBCPP_INLINE_VISIBILITY`.
> > > 
> > > That's loaded with history. Basically, `_LIBCPP_INLINE_VISIBILITY` is the same as `_LIBCPP_HIDE_FROM_ABI`, but `_LIBCPP_HIDE_FROM_ABI` is the newer and more descriptive spelling. In the past, it made sense to use `_LIBCPP_INLINE_VISIBILITY` because it was implemented with `__always_inline__`, but it's not anymore, so `_LIBCPP_HIDE_FROM_ABI` makes more sense. I didn't do a wholesale renaming of `_LIBCPP_INLINE_VISIBILITY` => `_LIBCPP_HIDE_FROM_ABI` because I wanted to avoid touching like 50% of the lines in the library.
> > > Basically, `_LIBCPP_INLINE_VISIBILITY` is the same as `_LIBCPP_HIDE_FROM_ABI`, but `_LIBCPP_HIDE_FROM_ABI` is the newer and more descriptive spelling. In the past, it made sense to use `_LIBCPP_INLINE_VISIBILITY` because it was implemented with `__always_inline__`, but it's not anymore, so `_LIBCPP_HIDE_FROM_ABI` makes more sense. I didn't do a wholesale renaming of `_LIBCPP_INLINE_VISIBILITY` => `_LIBCPP_HIDE_FROM_ABI` because I wanted to avoid touching like 50% of the lines in the library.
> > 
> > I agree it's not a good idea to do this conversion, unless we want to do other large changes touching the entire library; like running `clang-format` on all code. The only disadvantage of having most code using `_LIBCPP_INLINE_VISIBILITY` is that (new) contributors will probably emulate the wrong style. So maybe adding a note here about the state of `_LIBCPP_INLINE_VISIBILITY` would be a good idea. In the format library I've used `_LIBCPP_INLINE_VISIBILITY`, since I thought that was the proper way.
> > 
> > 
> I added documentation for `_LIBCPP_INLINE_VISIBILITY` and reworded the checklist as you suggested. Thanks for the comments!
Thanks the change looks good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106287