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

Mon Jul 19 11:34:39 PDT 2021

ldionne 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:
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106287