efriedma added a comment. In D81583#2139002 <https://reviews.llvm.org/D81583#2139002>, @uweigand wrote:
> In D81583#2137277 <https://reviews.llvm.org/D81583#2137277>, @efriedma wrote: > > > I'm tempted to say this is a bugfix for the implementation of > > no_unique_address, and just fix it globally for all ABIs. We're never > > going to get anything done here if we require a separate patch for each ABI > > variant clang supports. > > > Well, I can certainly do that. Would you prefer me to completely remove the > AllowNoUniqueAddress parameter, or keep it but default to true (so ABIs can > still override if necessary)? Just drop it; I don't expect any ABI would want to override it. ================ Comment at: clang/lib/CodeGen/TargetInfo.cpp:7245 // do count. So do anonymous bitfields that aren't zero-sized. - if (getContext().getLangOpts().CPlusPlus && - FD->isZeroLengthBitField(getContext())) - continue; + if (getContext().getLangOpts().CPlusPlus) { + if (FD->isZeroLengthBitField(getContext())) ---------------- uweigand wrote: > efriedma wrote: > > Only loosely relevant to this patch, but checking getLangOpts().CPlusPlus > > here seems weird; doesn't that break calling functions defined in C from > > C++ code? > I agree that this difference between C and C++ is weird, but it does match > the behavior of GCC. (Which is itself weird, but a long-standing accident > that we probably cannot fix without breaking existing code at this point.) > > Now, you bring up an interesting point: When C++ code calls a function > defined in C code, the C++ part would have to refer to an `extern "C"` > declaration. The correct thing to do would probably be to handle those > according to the C ABI rules, not the C++ rules, in this case where the two > differ. But currently GCC doesn't do that either. (But since that would be > broken anyway, I think we **can** fix that.) In any case, I agree that this > is really a separate problem, distinct from this patch. Okay. Please move the check for NoUniqueAddressAttr out of the CPlusPlus check; I don't think it's currently possible to use from C, but better to be on the safe side. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81583/new/ https://reviews.llvm.org/D81583 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
