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

2020-08-20 Thread Bevin Hansson via Phabricator via cfe-commits
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.

2020-08-19 Thread Eli Friedman via Phabricator via cfe-commits
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1847 +IsIncompleteArrayType ? CharUnits::Zero() : TI.first; +AlignIsRequired =

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

2020-08-19 Thread Bevin Hansson via Phabricator via cfe-commits
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.

2020-08-18 Thread Eli Friedman via Phabricator via cfe-commits
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.

2020-08-18 Thread Bevin Hansson via Phabricator via cfe-commits
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.

2020-08-06 Thread Richard Smith - zygoloid via Phabricator via cfe-commits
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.

2020-08-06 Thread Thorsten via Phabricator via cfe-commits
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.

2020-08-06 Thread Bevin Hansson via Phabricator via cfe-commits
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.

2020-08-05 Thread Eli Friedman via Phabricator via cfe-commits
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.

2020-08-05 Thread Bjorn Pettersson via Phabricator via cfe-commits
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.

2020-08-05 Thread Richard Smith - zygoloid via Phabricator via cfe-commits
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.

2020-08-05 Thread Bevin Hansson via Phabricator via cfe-commits
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.

2020-08-05 Thread Bevin Hansson via Phabricator via cfe-commits
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.

2020-08-04 Thread Richard Smith - zygoloid via Phabricator via cfe-commits
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.

2020-08-04 Thread Bevin Hansson via Phabricator via cfe-commits
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.

2020-08-04 Thread Xiangling Liao via Phabricator via cfe-commits
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.

2020-08-04 Thread Bevin Hansson via Phabricator via cfe-commits
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