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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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)) {}
----------------
Mordante wrote:
> 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".)
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.


================
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>{};
----------------
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.


================
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);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142332