Thanks, should hopefully be fixed by r364081. On Fri, 21 Jun 2019 at 01:12, Yvan Roux via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> Hi Richard, > > This commit broke ARM bots, logs are available here: > > > http://lab.llvm.org:8011/builders/clang-cmake-armv8-quick/builds/13576/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Atail-padding.cpp > > Thanks, > Yvan > > On Thu, 20 Jun 2019 at 23:06, John McCall via Phabricator via > llvm-commits <llvm-comm...@lists.llvm.org> wrote: > > > > rjmccall added a comment. > > > > In D63451#1552609 <https://reviews.llvm.org/D63451#1552609>, @rsmith > wrote: > > > > > In D63451#1549563 <https://reviews.llvm.org/D63451#1549563>, > @rjmccall wrote: > > > > > > > Can this attribute not be applied to a base class, or to a type? > > > > > > > > > The standard attribute forbids that right now. We could add a custom > attribute that permits it, but we're required to diagnose application of > the standard attribute to a type -- though a warning would suffice to > satisfy that requirement. (Because this gives "same as a base class" > layout, adding it to a base class wouldn't have any effect right now, but > we could certainly say that the attribute on a class type also implies the > class is not POD for the purpose of layout, to permit tail padding reuse.) > > > > > > I think we're talking about slightly different things. > > > > You're trying to make sure that fields can take advantage of the layout > optimizations available to base classes so that library code doesn't need > to contort to e.g. use the empty-base-class optimization just to avoid > wasting an entire pointer to store an empty allocator. This attribute > seems well-targeted for that. > > > > Totally separately from that — but perhaps better-related to the name of > the attribute — I know that some projects have, in the past, had surprising > problems with the rule that class layout can't place empty base classes at > the same offset as other objects of the same type. For example, IIRC > WebKit used to have a `Noncopyable` class that deleted its copy constructor > and copy-assignment operators, and there was an idiom to inherit from this > in order to disable copying, and at one point they discovered that some > fairly important class was quite a bit larger than expected because of this > rule — I think they were also using the base class redundantly throughout > the hierarchy because they didn't expect there to be any harm to doing so. > They would've no doubt appreciated the ability to just put the attribute on > `Noncopyable`. But of course they fixed that years ago, so I can't say > that it's a serious issue for them now. > > > > > > Repository: > > rL LLVM > > > > CHANGES SINCE LAST ACTION > > https://reviews.llvm.org/D63451/new/ > > > > https://reviews.llvm.org/D63451 > > > > > > > > _______________________________________________ > > llvm-commits mailing list > > llvm-comm...@lists.llvm.org > > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits