[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:2582-2584
+    int64_t BitfieldSize = Field->getBitWidthValue(Context);
+    if (BitfieldSize > FieldSizeInBits)
+      return llvm::None;
----------------
steakhal wrote:
> Why do you return `None` here?
These bits were just extracted from `structHasUniqueObjectRepresentations`, the underlying logic was not changed here. (originally in line 2615)
I believe this handles the case "If the width of a bit-field is larger than the width of the bit-field's type (or, in case of an enumeration type, of its underlying type), the extra bits are padding bits" from [class.bit]


================
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) {
----------------
steakhal wrote:
> 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.
This function is also called with (non virtual) base class subobjects, not just fields.
A `None` return value indicates that the subobjects do not have unique object representations, otherwise the size of the subobjects is returned. This allows us to detect for example padding between a base class and the first field. 


================
Comment at: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp:42
+} // namespace TailPaddingReuse
+static_assert(__has_unique_object_representations(TailPaddingReuse::B));
----------------
steakhal wrote:
> Why is this outside of the namespace declaration?
Tbh I'm not sure. I can move it in the namespace if you think it'd be better there. 


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

https://reviews.llvm.org/D89649