hubert.reinterpretcast added inline comments.
================
Comment at: clang/lib/AST/ASTContext.cpp:2424
+ (T->isSpecificBuiltinType(BuiltinType::LongDouble) &&
+ Target->supportsAIXPowerAlignment()))
// Don't increase the alignment if an alignment attribute was specified on
a
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > Does `supportsAIXPowerAlignment` express the condition we want to check
> > here? That might be true for an implementation operating with `mac68k`
> > alignment rules.
> Yeah, `supportsAIXPowerAlignment` cannot separate the preferred alignment of
> double, long double between `power/natural` and `mac68k` alignment rules. But
> I noticed that currently, AIX target on wyvern or XL don't support `mac68k` ,
> so maybe we should leave further changes to the patch which is gonna
> implement `mac68k` alignment rules? The possible solution I am thinking is we
> can add checking if the decl has `AlignMac68kAttr` into query to separate
> things out.
>
> Another thing is that once we start supporting mac68k alignment rule(if we
> will), should we also change the ABI align values as well? (e.g. for double,
> it should be 2 instead)
If the "base state" is AIX `power` alignment for a platform, I suggest that the
name be `defaultsToAIXPowerAlignment`.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:660
+ /// When there are OverlappingEmptyFields existing in the aggregate, the
+ /// flag shows if the following first non-overlappingEmptyField has been
+ /// handled, if any.
----------------
Xiangling_L wrote:
> hubert.reinterpretcast wrote:
> > I suggest to replace (if correct) "non-overlappingEmptyField" with
> > "non-empty or empty-but-non-overlapping field".
> >
> Thanks for your suggestion. But I kinda prefer using
> `NonOverlappingEmptyField`, it is more consistent with
> `IsOverlappingEmptyField`. And also the equivalent name to
> `NonOverlappingEmptyField` is `NonOverlappingAndNonEmptyField` which is
> tedious I think.
I am suggesting a change to the comment and not the name here. If both the
comment and the name uses the same (possibly confusing) form to express the
concept, then the comment does not aid comprehension of the code.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79719/new/
https://reviews.llvm.org/D79719
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
