We already have a clang-tidy check for making sure that _LIBCPP_HIDE_FROM_ABI is on free functions. This patch extends this to class members. The places where we don't check for _LIBCPP_HIDE_FROM_ABI are classes for which we have an instantiation in the library.
Details
- Reviewers
ldionne Mordante var-const jdoerfert - Group Reviewers
Restricted Project - Commits
- rG83ce13972126: [libc++] Add hide_from_abi check for classes
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
In general I like it, for reviewing it would have been nicer to split the patch in multiple parts:
- removal from trivial functions
- adding on non-trivial functions
That would also make it easier to see which part of the clang-tidy enables which test.
Also please don't make unrelated changes in the library code.
libcxx/include/__chrono/day.h | ||
---|---|---|
32 | Is it an issue to have this unneeded _LIBCPP_HIDE_FROM_ABI? I ask since when the class becomes non-trivial we need to add it again. | |
libcxx/include/__ranges/iota_view.h | ||
51 | Please don't hide these kind of changes in this patch. | |
libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp | ||
82 | why only testing for trivially copyable? |
libcxx/include/__chrono/day.h | ||
---|---|---|
32 | Not really. I wouldn't expect internal classes to be a problem either. It's not like we regularly change a class to be trivial which wasn't trivial before. The main reason I added this check is for tag classes and the like where it's just noise, e.g. struct some_tag { explicit some_tag() = default; } shouldn't have a _LIBCPP_HIDE_FROM_ABI IMO. | |
libcxx/include/__ranges/iota_view.h | ||
51 | What do you mean with "these kinds of changes"? I didn't think this would be a problem, given that it will never be executed anyways. | |
libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp | ||
82 | I'm not sure what you mean. The name is wrong, but I check for isTrivial(), not isTriviallyCopyable(). Was it just about the name being wrong? |
I am personally find with this patch after a few minor changes outlined in my comments. I think this is a large mechanical patch and splitting it might or might not help reviewing, I am not sure. I went through 90% of the mechanical changes to validate them and everything looked fine to me. Will let Mark LGTM the patch since he had comments.
libcxx/include/__algorithm/comp_ref_type.h | ||
---|---|---|
18 | Can your commit message please explain a bit more what the patch is about? | |
libcxx/include/__ranges/iota_view.h | ||
51 | So there were two choices here:
I think this patch should add _LIBCPP_HIDE_FROM_ABI because the reviewer's assumption is that all these changes are 100% mechanical, and this violates that assumption. We should then (trivially) make it consteval in a separate one-liner patch, cause I agree that's probably what it should have been in the first place. Or, alternatively, add consteval as a NFC prior to this patch. | |
libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp | ||
59 | ||
82 | ||
94 |
- Address comments
- Try to fix CI
- Rebased
libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp | ||
---|---|---|
98 | I plan to merge abi_tag_on_virtual into hide_from_abi. I'd rather improve the error messages there, since I probably have to modify this part anyways. |
libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp | ||
---|---|---|
98 | That's fine by me too. I just like to have a better error message since it's trivial to do. |
Can your commit message please explain a bit more what the patch is about?