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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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.



================
Comment at: libcxx/include/__chrono/day.h:32
 public:
-    _LIBCPP_HIDE_FROM_ABI day() = default;
     _LIBCPP_HIDE_FROM_ABI explicit inline constexpr day(unsigned __val) noexcept : __d_(static_cast<unsigned char>(__val)) {}
----------------
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".)


================
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>{};
----------------
Please don't hide these kind of changes in this patch. 


================
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);
----------------
why only testing for trivially copyable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142332