This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add a bunch of missing _LIBCPP_HIDE_FROM_ABI in <ranges>
ClosedPublic

Authored by ldionne on Jul 19 2021, 9:38 AM.

Details

Summary

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.

Diff Detail

Event Timeline

ldionne requested review of this revision.Jul 19 2021, 9:38 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2021, 9:38 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 359839.Jul 19 2021, 10:32 AM

Fix a few failures with [[nodiscard]] interaction

Mordante added inline comments.
libcxx/docs/Contributing.rst
34

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.

ldionne added inline comments.Jul 19 2021, 11:34 AM
libcxx/docs/Contributing.rst
34

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.

Mordante added inline comments.Jul 19 2021, 12:27 PM
libcxx/docs/Contributing.rst
34

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.

ldionne accepted this revision.Jul 19 2021, 4:34 PM
ldionne marked 2 inline comments as done.
ldionne added inline comments.
libcxx/docs/Contributing.rst
34

I added documentation for _LIBCPP_INLINE_VISIBILITY and reworded the checklist as you suggested. Thanks for the comments!

This revision is now accepted and ready to land.Jul 19 2021, 4:34 PM
This revision was automatically updated to reflect the committed changes.
ldionne marked an inline comment as done.
Mordante added inline comments.Jul 20 2021, 10:02 AM
libcxx/docs/Contributing.rst
34

Thanks the change looks good!