stevewan marked an inline comment as done.
stevewan added inline comments.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1984
+ auto checkTypeIsTypedef = [](auto &&checkTypeIsTypedef,
+ const auto Ty) -> bool {
+ bool isTypedef = false;
----------------
rjmccall wrote:
> Hah. The knot-tying here is really cute, but I think either just make it a
> static function somewhere in the file, or make it iterative instead of
> recursive.
>
> ...actually, I think it would be better to just make the computation in
> `getTypeInfo` preserve more information. Make `TypeInfo` carry a
> three-valued enum instead of an `AlignIsRequired` flag:
>
> ```
> enum class AlignRequirement {
> /// The alignment was not explicit in code.
> None,
>
> /// The alignment comes from an alignment attribute on a typedef.
> RequiredByTypedef,
>
> /// The alignment comes from an alignment attribute on a record type.
> RequiredByRecord,
> };
> ```
>
> You can add a method on `TypeInfo` to ask if the alignment is required for
> any reason and make all the existing clients use that instead.
Good idea. I had thought about a similar approach but was afraid that the
change may get too pervasive. I think it turned out fine. I'm now debating
whether I should split this whole bool-to-enum change into a separate patch,
any preference?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107394/new/
https://reviews.llvm.org/D107394
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
