[clang] [Clang] Fix missing diagnostic for non-standard layout type in `offsetof` (PR #65246)

Louis Dionne via cfe-commits cfe-commits at lists.llvm.org
Mon Sep 25 06:18:40 PDT 2023


ldionne wrote:

> Thank you for your review! Here are some comments and my thoughts.
> 
> > Note that you should probably rebase your patch onto main and force-push to update the PR
> 
> Does it mean that I can rebase and force-push since (it should be avoided in normal case but) this is a special case?

That's what I usually do. Do we have any Github workflow documentation that mentions we shouldn't force-push to update PRs? If so, I am not aware of it.

> 
> > So... should we strive to eradicate this from libc++ in the first place, or should we just silence the warning. WDYT?
> 
> I think it's good to only silence the warning and (if necessary) leave some TODO comments about the `offsetof` usage because it's undesirable for this patch to be larger than it needs to be..

My question is about the desired end state of libc++, not whether that end state should be achieved in this specific patch or not.

> 
> > Also, given that it is conditionally supported in C++17, I assume this warning doesn't trigger at all in C++17 mode, right?
> 
> I believe the purpose of this warning is to tell you that your program works now, but may not be compiled with another compiler. So the similar warning should appear even in C++17 mode. I chedked and found that `_FirstPaddingByte<>` is ~standard layout but non-POD~ both standard-layout and POD (I checked them with `std::is_standard_layout` and `std::is_pod`). Using `offsetof` with a standard-layout class is supported in C++17, so I guess there is no warning

What `T` type did you use to check that `_FirstPaddingByte<>` was standard layout and POD? Certainly `_FirstPaddingByte<>` can't be standard layout if the `T` it holds is not standard layout?

@philnik777 do you have thoughts on this?

https://github.com/llvm/llvm-project/pull/65246


More information about the cfe-commits mailing list