[PATCH] D54472: Disable invalid isPodLike<> specialization

[serge via Phabricator via llvm-commits]

serge-sans-paille added inline comments.


================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:188
 
-    BlockNode() = default;
+    BlockNode() noexcept(true) = default;
     BlockNode(IndexType Index) : Index(Index) {}
----------------
Quuxplusone wrote:
> Unintended diff? Also, surely `noexcept(true)` a.k.a. `noexcept` would be the default here anyway. If anything wants `noexcept`, it'd be line 189, not line 188. :)
That's unfortunate but intended: for some reason, and for some clang version, the type trait ``is_trivially_copyable`` leads to this being required. There's no such issue with the gcc version I compiled clang with.


================
Comment at: include/llvm/Support/type_traits.h:163
+      (has_trivial_copy_constructor || has_trivial_copy_assign ||
+       has_trivial_move_constructor || has_trivial_move_assign);
+
----------------
Quuxplusone wrote:
> This last clause — `(has_trivial_copy_constructor || has_trivial_copy_assign || has_trivial_move_constructor || has_trivial_move_assign)` — does not correspond to anything in the Standard. So:
> 
> https://godbolt.org/z/R5hCff
> ```
> struct S {
>     S(S&&) = delete;
> };
> static_assert(is_trivially_copyable<S>::value);
> ```
> ```
> error: static_assert failed "consistent behavior"
>   static_assert(value == std::is_trivially_copyable<T>::value, "consistent behavior");
>   ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ```
> I'm not sure but I would suggest just getting rid of this last clause.
>From https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable (yeah that's not the standard, but I do hope it's derived from the standard)

> at least one copy constructor, move constructor, copy assignment operator, or move assignment operator is non-deleted

That's the intent I was trying to capture.

I'm okay with getting rid of this capture, especially when looking at https://godbolt.org/z/p3-Cy4.


================
Comment at: tools/clang/lib/Sema/SemaChecking.cpp:11553
     public:
-      Seq() = default;
+      Seq() : Index(0) {}
     };
----------------
Quuxplusone wrote:
> Unintended diff? But harmless.
Same as above, omitting this change  leads to error in type traits instantiation for some compiler version.


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

https://reviews.llvm.org/D54472