[libcxx-commits] [PATCH] D142332: [libc++] Add hide_from_abi check for classes

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: ldionne.
ldionne added a comment.

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.



================
Comment at: libcxx/include/__algorithm/comp_ref_type.h:18
 #  pragma GCC system_header
 #endif
 
----------------
Can your commit message please explain a bit more what the patch is about?


================
Comment at: libcxx/include/__ranges/iota_view.h:48
   struct __get_wider_signed {
-    static auto __call() {
+    consteval static auto __call() {
            if constexpr (sizeof(_Int) < sizeof(short)) return type_identity<short>{};
----------------
philnik wrote:
> Mordante wrote:
> > Please don't hide these kind of changes in this patch. 
> 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.
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.


================
Comment at: libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp:59
+
+  // TODO: find a better way to chek for explicit instantiations. Currently, every template that has a full specialization
+  // is ignored. For example, vector is ignored because we instantiate vector<double> in discrete_distribution.
----------------



================
Comment at: libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp:82
+  finder->addMatcher(cxxMethodDecl(has_hide_from_abi_attr, on_trivial)
+                         .bind("hide_from_abi_on_defaulted_smf_in_trivially_copyable_class"),
+                     this);
----------------
philnik wrote:
> Mordante wrote:
> > why only testing for trivially copyable?
> 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?



================
Comment at: libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp:94
+      call != nullptr) {
+    diag(call->getLocation(), "_LIBCPP_HIDE_FROM_ABI is missing");
+  }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142332