compnerd added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1948
+def RequireDesignatedInit : InheritableAttr {
+ let Spellings = [GNU<"require_designated_init">];
+ let Subjects = SubjectList<[Type]>;
----------------
This seems like an extension? I think that we want to explicitly mark this as
an extension, as this is not available in GCC AFAIK.
================
Comment at: clang/include/clang/Basic/Attr.td:1953
+ let Documentation = [RequireDesignatedInitDocs];
+}
+
----------------
Is this meant for C or C++? Given that the docs reference the C++20 designated
initialization, I suspect that this is meant for C++, in which case, we should
restrict this to the C++ language. At which point, perhaps we should restrict
the spelling to the C++ attribute syntax as well?
================
Comment at: clang/include/clang/Basic/Attr.td:1956
+def Required : InheritableAttr {
+ let Spellings = [GNU<"required">];
+ let Subjects = SubjectList<[Field]>;
----------------
This seems like an extension as well? I’m not sure that the spelling here is
good. It seems overly general. At the very least, something like
`__attribute__((__designated_initialization_required__))` seems better.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1453
+variable of that struct's type is declared, the declaration uses `designated
+initializer syntax
<https://en.cppreference.com/w/cpp/language/aggregate_initialization#Designated_initializers>`_.
+For a struct ``Foo`` with one integer field ``x``, the following declarations
----------------
I think it would be better to reference the specification instead of a link to
cppreference. Also, I think its better to use the language agnostic URL.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:11214
+ if (TagDecl *TD = VDecl->getType().getTypePtr()->getAsTagDecl()) {
+ // If the type of the declaration is a struct/class and that type has the
----------------
I would just use `auto`, you spell out the type in the expression.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:11218
+ // the proper designated initializer syntax.
+ if (TD && TD->hasAttr<RequireDesignatedInitAttr>()) {
+ if (InitListExpr *ILE = dyn_cast<InitListExpr>(Init)) {
----------------
`TD &&` is tautologically true. Please drop it.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:11219
+ if (TD && TD->hasAttr<RequireDesignatedInitAttr>()) {
+ if (InitListExpr *ILE = dyn_cast<InitListExpr>(Init)) {
+ for (unsigned i = 0; i < ILE->getNumInits(); i++) {
----------------
I would use `auto` as you spell out the type in the expression.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:11223
+ DesignatedInitExpr *DIE = dyn_cast<DesignatedInitExpr>(init);
+ if (!DIE) {
+ SourceRange SR(VDecl->getSourceRange().getBegin(),
----------------
I would just combine this with the previous like and unindent the rest of the
path by using the following:
```
if (!isa<DesignatedInitExpr>(init))
continue;
```
================
Comment at: clang/lib/Sema/SemaDecl.cpp:11238
+ }
+ // If the type of the declaration is a struct/class, we must check whether
+ // any of the fields have the required attribute. If any of them do, we
must
----------------
A new line before this line would be nice.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:11247
+ std::set<IdentifierInfo *> RequiredFields;
+ for (auto FD : RD->fields()) {
+ if (FD->hasAttr<RequiredAttr>()) {
----------------
Any reason that this cannot be `const auto *FD`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64380/new/
https://reviews.llvm.org/D64380
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
