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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne marked 2 inline comments as done.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
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:
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106287