We've been forgetting to add those to most of the <ranges> review.
To avoid forgetting in the future, I added an item in the pre-commit
checklist.
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rGd153e7d0a5f2: [libc++] Add a bunch of missing _LIBCPP_HIDE_FROM_ABI in <ranges>
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/docs/Contributing.rst | ||
---|---|---|
34 | Shouldn't this be more general? 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. |
libcxx/docs/Contributing.rst | ||
---|---|---|
34 |
Hmm, yes, I agree. Will fix.
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. |
libcxx/docs/Contributing.rst | ||
---|---|---|
34 |
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. |
libcxx/docs/Contributing.rst | ||
---|---|---|
34 | I added documentation for _LIBCPP_INLINE_VISIBILITY and reworded the checklist as you suggested. Thanks for the comments! |
libcxx/docs/Contributing.rst | ||
---|---|---|
34 | Thanks the change looks good! |
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.