[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

It looks great.



================
Comment at: clang/lib/AST/ASTContext.cpp:2582-2584
+    int64_t BitfieldSize = Field->getBitWidthValue(Context);
+    if (BitfieldSize > FieldSizeInBits)
+      return llvm::None;
----------------
Why do you return `None` here?


================
Comment at: clang/lib/AST/ASTContext.cpp:2595-2598
+template <typename RangeT>
+static llvm::Optional<int64_t> structSubobjectsHaveUniqueObjectRepresentations(
+    const RangeT &Subobjects, int64_t CurOffsetInBits, const ASTContext &Context,
+    const clang::ASTRecordLayout &Layout) {
----------------
Why is this a template?
Can't you just take the `field_range`?

By the same token, `structSubobjectsHaveUniqueObjectRepresentations` returns an `optional int`. I'm confused.


================
Comment at: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -std=c++2a %s
+//  expected-no-diagnostics
----------------
gbencze wrote:
> Just to be sure: is the specifying the triple here enough to ensure that this always uses the Itanium ABI? I believe MSVC currently ignores the `no_unique_address` attribute. Or do I need to add some more flags?
> Alternatively, the static_asserts could be modified to check `sizeof(T) > [expected size with Itanium ABI] || __has_unique_object_representations(T)`
I think it's better to specify the triple exactly 


================
Comment at: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp:42
+} // namespace TailPaddingReuse
+static_assert(__has_unique_object_representations(TailPaddingReuse::B));
----------------
Why is this outside of the namespace declaration?


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

https://reviews.llvm.org/D89649