[PATCH] D64380: Add 'require_designated_init' and 'required' attribute to clang

[Saleem Abdulrasool via Phabricator via cfe-commits]

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