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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
