This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add hide_from_abi check for classes
ClosedPublic

Authored by philnik on Jan 23 2023, 1:27 AM.

Details

Summary

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.

Diff Detail

Event Timeline

philnik created this revision.Jan 23 2023, 1:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 1:27 AM
philnik requested review of this revision.Jan 23 2023, 1:27 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 491355.Jan 23 2023, 6:51 AM

Try to fix CI

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.
(Obviously we can't just change this class, but it would apply to internal classes that are not user "visible".)

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?

philnik added inline comments.Jan 30 2023, 10:56 AM
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?

ldionne accepted this revision as: ldionne.Mar 2 2023, 9:32 AM

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:

  1. Add _LIBCPP_HIDE_FROM_ABI, or
  2. add consteval to shut up the clang-tidy check

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
philnik edited the summary of this revision. (Show Details)Mar 5 2023, 5:07 AM
philnik updated this revision to Diff 502430.Mar 5 2023, 5:15 AM
philnik marked 7 inline comments as done.

Address comments

philnik updated this revision to Diff 503444.Mar 8 2023, 10:53 AM

Try to fix CI

philnik updated this revision to Diff 503449.Mar 8 2023, 11:03 AM

More fixes

philnik updated this revision to Diff 506374.Mar 19 2023, 3:56 AM

Try to fix CI

philnik updated this revision to Diff 512164.Apr 10 2023, 8:44 AM

Try to fix CI

philnik updated this revision to Diff 512210.Apr 10 2023, 12:01 PM

Try to fix CI

Mordante accepted this revision.Apr 14 2023, 10:55 AM

LGTM modulo some nits.

libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
92

Could you add an example in the comment.

98

How about testing whether the method is virtual and show the proper message. This can probabl be done with a '%select' in the diagnotic message.

This revision is now accepted and ready to land.Apr 14 2023, 10:55 AM
philnik updated this revision to Diff 513906.Apr 15 2023, 7:50 AM
philnik marked an inline comment as done.
  • 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.

Mordante added inline comments.Apr 15 2023, 8:06 AM
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.

philnik updated this revision to Diff 513920.Apr 15 2023, 9:04 AM

Try to fix CI

philnik updated this revision to Diff 513955.Apr 15 2023, 7:19 PM

Try to fix CI

philnik updated this revision to Diff 513960.Apr 15 2023, 8:53 PM

Try to fix CI

This revision was landed with ongoing or failed builds.Apr 16 2023, 6:23 AM
This revision was automatically updated to reflect the committed changes.