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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1947
 
+def RequireDesignatedInit : InheritableAttr {
+  let Spellings = [CXX11<"clang", "require_designated_init">];
----------------
This should be `RequiresDesignator`.


================
Comment at: clang/include/clang/Basic/Attr.td:1948
+def RequireDesignatedInit : InheritableAttr {
+  let Spellings = [CXX11<"clang", "require_designated_init">];
+  let Subjects = SubjectList<[Type]>;
----------------
I'd prefer this be named `requires_designator`.


================
Comment at: clang/include/clang/Basic/Attr.td:1949
+  let Spellings = [CXX11<"clang", "require_designated_init">];
+  let Subjects = SubjectList<[Type]>;
+  let Documentation = [RequireDesignatedInitDocs];
----------------
I'm a bit confused by this subject -- it's a declaration attribute that appertains to types? Why is this not `RecordDecl` instead?


================
Comment at: clang/include/clang/Basic/Attr.td:1953
+
+def Required : InheritableAttr {
+  let Spellings = [CXX11<"clang", "designated_init_required">];
----------------
This should be `RequiresInit`


================
Comment at: clang/include/clang/Basic/Attr.td:1954
+def Required : InheritableAttr {
+  let Spellings = [CXX11<"clang", "designated_init_required">];
+  let Subjects = SubjectList<[Field]>;
----------------
This should also use the `Clang` spelling. Also, I'd prefer this be named `requires_init`.

As it stands, these two attribute names are *way* too similar.


================
Comment at: clang/include/clang/Basic/Attr.td:1948
+def RequireDesignatedInit : InheritableAttr {
+  let Spellings = [GNU<"require_designated_init">];
+  let Subjects = SubjectList<[Type]>;
----------------
compnerd wrote:
> 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.
This should use the `Clang` attribute spelling rather than `CXX11`.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1448
 
+def RequireDesignatedInitDocs : Documentation {
+  let Category = DocCatType;
----------------
The behavior should be documented as to what happens when you apply these attributes to unions.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1451
+  let Content = [{
+This attribute can be applied to a struct definition to require that anytime a
+variable of that struct's type is declared, the declaration uses designated
----------------
anytime -> any time


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1452
+This attribute can be applied to a struct definition to require that anytime a
+variable of that struct's type is declared, the declaration uses designated
+initializer syntax ([dcl.init.aggr]p3.1).
----------------
struct's -> structure's


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1484
+This attribute can be applied to a field definition within a struct or a class
+to require that anytime a variable of the struct/class's type is declared, that
+field must be initialized using designated initializer syntax ([dcl.init.aggr]p3.1).
----------------
anytime -> any time
struct/class -> struct


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3533-3541
+def err_require_designated_init_failed : Error<
+  "variable declaration does not use designated initializer syntax">;
+def note_declared_required_designated_init_here : Note<
+  "required by 'require_designated_init' attribute here">;
+
+def err_required_failed : Error<
+  "initializer for variable %0 must explicitly initialize field %1">;
----------------
I'm a little uncomfortable with these being errors rather than warnings. I think of these attributes as being preferences rather than requirements; the code isn't *wrong* if it fails to use the designated initializer (it's conforming to C or C++, does not have UB, etc) and other implementations are free to ignore those attributes and the end result will be identical to what's produced by Clang.

What do you think about turning these into warnings? Users can always use `-Werror` to strengthen their own requirements.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11214
 
+  if (auto *TD = VDecl->getType().getTypePtr()->getAsTagDecl()) {
+    // If the type of the declaration is a struct/class and that type has the
----------------
Any reason this shouldn't be `const auto *`? (Propagated elsewhere.)


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11220-11221
+      if (auto *ILE = dyn_cast<InitListExpr>(Init)) {
+        for (unsigned i = 0; i < ILE->getNumInits(); i++) {
+          Expr *init = ILE->getInit(i);
+          if (auto *DIE = dyn_cast<DesignatedInitExpr>(init))
----------------
You can use a range-based for loop over `inits()`, I believe. Also, the variable should be named `Init` to meet the coding style.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11228
+              << SR;
+          auto attr = TD->getAttr<RequireDesignatedInitAttr>();
+          Diag(attr->getLocation(),
----------------
Rather than use `hasAttr<>` and `getAttr<>` like this, you should only use `getAttr<>` above. e.g., `if (const auto *RAttr = TD->getAttr<RequireDesignatedInitAttr>())`.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11255
+      if (RequiredFields.size() > 0) {
+        if (InitListExpr *ILE = dyn_cast<InitListExpr>(Init)) {
+          for (unsigned I = 0, E = ILE->getNumInits(); I != E; I++) {
----------------
You can use `const auto *` here.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11256-11257
+        if (InitListExpr *ILE = dyn_cast<InitListExpr>(Init)) {
+          for (unsigned I = 0, E = ILE->getNumInits(); I != E; I++) {
+            Expr *init = ILE->getInit(I);
+            if (DesignatedInitExpr *DIE = dyn_cast<DesignatedInitExpr>(init)) {
----------------
`inits()` and `Init`, as above.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11258
+            Expr *init = ILE->getInit(I);
+            if (DesignatedInitExpr *DIE = dyn_cast<DesignatedInitExpr>(init)) {
+              DesignatedInitExpr::Designator *D = DIE->getDesignator(0);
----------------
`const auto *`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11261
+              if (D->isFieldDesignator()) {
+                IdentifierInfo *name = D->getFieldName();
+                if (RequiredFields.count(name) != 0)
----------------
`Name` per coding styles.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11270
+        // each field.
+        for (auto FD : RD->fields()) {
+          if (RequiredFields.count(FD->getIdentifier()) != 0) {
----------------
`const auto *`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11276
+                << SR << VDecl->getName() << FD->getName();
+            auto attr = FD->getAttr<RequiredAttr>();
+            Diag(attr->getLocation(), diag::note_declared_required_here)
----------------
`const auto *A` (naming conventions and not conflicting with a type name).


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11631
 
+    if (TagDecl *TD = Type.getTypePtr()->getAsTagDecl()) {
+      // If the type of the declaration is a struct/class and that type has the
----------------
`const auto *`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11634-11637
+      if (TD->hasAttr<RequireDesignatedInitAttr>()) {
+        Diag(Var->getLocation(), diag::err_require_designated_init_failed)
+            << Var->getSourceRange();
+        auto attr = TD->getAttr<RequireDesignatedInitAttr>();
----------------
Don't do `hasAttr<>` followed by `getAttr<>`, also watch the naming conventions.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11648
+      // syntax.
+      if (RecordDecl *RD = dyn_cast<RecordDecl>(TD)) {
+        for (auto FD : RD->fields()) {
----------------
`const auto *`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11649
+      if (RecordDecl *RD = dyn_cast<RecordDecl>(TD)) {
+        for (auto FD : RD->fields()) {
+          if (FD->hasAttr<RequiredAttr>()) {
----------------
`const auto *`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:11650-11653
+          if (FD->hasAttr<RequiredAttr>()) {
+            Diag(Var->getLocation(), diag::err_required_failed)
+                << Var->getSourceRange() << Var->getName() << FD->getName();
+            auto attr = FD->getAttr<RequiredAttr>();
----------------
`hasAttr<>` followed by `getAttr<>`, as above.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5312-5327
+static void handleRequireDesignatedInit(Sema &S, Decl *D,
+                                        const ParsedAttr &AL) {
+  if (TagDecl *TD = dyn_cast<TagDecl>(D))
+    TD->addAttr(::new (S.Context) RequireDesignatedInitAttr(
+        AL.getRange(), S.Context, AL.getAttributeSpellingListIndex()));
+  else
+    S.Diag(AL.getLoc(), diag::warn_attribute_ignored) << AL.getName();
----------------
Currently, both of these functions aren't needed (once you fix the code in Attr.td) because the checking is handled automatically for you. You can use `handleSimpleAttribute()` instead, from within the `switch` below.


================
Comment at: clang/test/Misc/pragma-attribute-supported-attributes-list.test:125
 // CHECK-NEXT: RequireConstantInit (SubjectMatchRule_variable_is_global)
+// CHECK-NEXT: Required (SubjectMatchRule_field)
 // CHECK-NEXT: Restrict (SubjectMatchRule_function)
----------------
I think you're missing the changes for the other attribute.


================
Comment at: clang/test/SemaCXX/attr-designated-init-required.cpp:63
+Cla c7{.x = 1, .y = 2};
+Cla c8{.x = 1, 2};
----------------
What should happen in these sort of situations?
```
class C {
  ATTR int x; // This field is private
};

struct S {
  ATTR int x;
};

struct D : private S { // S::x is private when referenced through D
};
```
If we're ignoring the attribute, we should diagnose it as being ignored. But I'm also not certain how friendship should interact with this.


================
Comment at: clang/test/SemaCXX/attr-designated-init-required.cpp:3
+
+#define ATTR [[clang::designated_init_required]]
+
----------------
emmettneyman wrote:
> compnerd wrote:
> > Why the macro?
> I modeled this file after `test/SemaCXX/attr-require-constant-initialization.cpp` where a macro is defined at the top. I assume just to keep things neat.
FWIW, I prefer seeing the attribute used explicitly. The macro is more of a distraction.


================
Comment at: clang/test/SemaCXX/attr-require-designated-init.cpp:3
+
+#define ATTR [[clang::require_designated_init]]
+
----------------
emmettneyman wrote:
> compnerd wrote:
> > Why the macro?
> See above.
Same concerns here as above.


================
Comment at: clang/test/SemaCXX/attr-require-designated-init.cpp:104
+Cla c11 = {.x = 1, .y = 2};
+Cla c12 = {.y = 2, .x = 1};
----------------
I think this should ignore fields that cannot be initialized, as in:
```
class C {
  int a;
protected:
  int b;
public:
  int c;
};

C c{.c = 12}; // Don't complain about a or b.
```
I'd also like to see more involved situations, like classes with inherited fields, privately inherited classes, etc. Also, some tests with unions would be appreciated as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64380/new/

https://reviews.llvm.org/D64380