[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1e7ec4842c1a: [AST] Get field size in chars rather than bits in RecordLayoutBuilder. (authored by ebevhan). Repository: rG LLVM Github Monorepo

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

ebevhan added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1847 +IsIncompleteArrayType ? CharUnits::Zero() : TI.first; +AlignIsRequired = Context.getTypeInfo(D->getType()).AlignIsRequired; }; efriedma wrote: > Can we fix

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

efriedma added a comment. I'm still concerned your approach to the computation of getTypeSize() is a ticking time bomb, but I'll take the cleanup even if the underlying motivation doesn't really make sense. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1847 +

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

ebevhan added a comment. It doesn't feel like this patch got a very positive reception, but I'd still like to try a bit more to get it in. Even though it's difficult to test this particular change upstream, would it still be acceptable to take the patch since it reverts the behavior to what it

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

rsmith added a comment. In D85191#2199574 , @ebevhan wrote: > In D85191#2197663 , @efriedma wrote: > >>> If the intent is for getTypeInfo to always return sizes that are multiples >>> of the char size, then the

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

tschuett added a comment. I don't know whether the name of your downstream target is a secret. Wouldn't it help you to add a fake 16bit per char target and add units tests to prevent regressions? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

ebevhan added a comment. In D85191#2197550 , @bjope wrote: > In D85191#2196863 , @rsmith wrote: > >> In D85191#2195923 , @ebevhan wrote: >> >>> In D85191#2193645

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

efriedma added a comment. > If the intent is for getTypeInfo to always return sizes that are multiples of > the char size, then the design should be inverted and getTypeInfo should > simply be calling getTypeInfoInChars and multiply that result by the char > size. But that isn't how it works.

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

bjope added a comment. In D85191#2196863 , @rsmith wrote: > In D85191#2195923 , @ebevhan wrote: > >> In D85191#2193645 , @rsmith wrote: >> This is not ideal, since it

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

rsmith added a comment. In D85191#2195923 , @ebevhan wrote: > In D85191#2193645 , @rsmith wrote: > >>> This is not ideal, since it makes the calculations char size dependent, and >>> breaks for sizes that are not

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

ebevhan added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1841 auto setDeclInfo = [&](bool IsIncompleteArrayType) { -TypeInfo TI = Context.getTypeInfo(D->getType()); -FieldAlign = Context.toCharUnitsFromBits(TI.Align); +auto TI =

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

ebevhan added a comment. In D85191#2193645 , @rsmith wrote: >> This is not ideal, since it makes the calculations char size dependent, and >> breaks for sizes that are not a multiple of the char size. > > How can we have a non-bitfield member whose size

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

rsmith added a comment. > This is not ideal, since it makes the calculations char size dependent, and breaks for sizes that are not a multiple of the char size. How can we have a non-bitfield member whose size is not a multiple of the size of a char? Repository: rG LLVM Github Monorepo

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

ebevhan added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1841 auto setDeclInfo = [&](bool IsIncompleteArrayType) { -TypeInfo TI = Context.getTypeInfo(D->getType()); -FieldAlign = Context.toCharUnitsFromBits(TI.Align); +auto TI =

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

Xiangling_L added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1841 auto setDeclInfo = [&](bool IsIncompleteArrayType) { -TypeInfo TI = Context.getTypeInfo(D->getType()); -FieldAlign = Context.toCharUnitsFromBits(TI.Align); +auto TI =

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

ebevhan created this revision. ebevhan added reviewers: jasonliu, efriedma. Herald added a project: clang. Herald added a subscriber: cfe-commits. ebevhan requested review of this revision. In D79719 , LayoutField was refactored to fetch the size of field types