[PATCH] D59754: [Sema] Add c++2a designated initializer warnings

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I think you're missing the enforcement of the rule that the same field name cannot be designated multiple times in a single //designated-initializer-list//. I'm fine with that being done separately (not as part of this patch), though.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:158
+def ext_c20_designated_init : Extension<
+  "C99 %0 designated initializers are a C++20 extension">, InGroup<C99>;
 def ext_designated_init : Extension<
----------------
See http://clang.llvm.org/docs/InternalsManual.html#the-format-string -- do not pass English text as arguments to diagnostics as this interferes with localization. Use `%select` instead, or use different diagnostics for the different kinds of problem.

I think use of multiple diagnostics would be preferable anyway, as it'd give you room to explain the nature of the problem better (for instance, rather than "mixed designated initializers", you could say "mixing designated and non-designated initializers in the same initializer list is a C99 feature not permitted in C++" or something like that).

Also, we use "X is a C++20 extension" to mean "this is a C++20 feature but you don't have C++20 enabled", which is very different from what you're using it to mean here. "ISO C++20 does not support X" is how we'd usually phrase "X is not a feature of C++20 but we know what you mean".


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:159-160
+  "C99 %0 designated initializers are a C++20 extension">, InGroup<C99>;
 def ext_designated_init : Extension<
   "designated initializers are a C99 feature">, InGroup<C99>;
 def err_array_designator_negative : Error<
----------------
In C++ <=17 mode, if we see a designated initializer that would be valid in C++20, we should diagnose that designated initializers are a C++20 feature, not that they're a C99 feature.


================
Comment at: clang/lib/Sema/SemaInit.cpp:2069-2072
+    if (!VerifyOnly && HasDesignatedInit && SemaRef.getLangOpts().CPlusPlus2a) {
+      SemaRef.Diag(Init->getBeginLoc(), diag::ext_c20_designated_init)
+          << "mixed" << Init->getSourceRange();
+    }
----------------
I think it would be preferable to diagnose the "mixed" case in the parser rather than here (it's a grammatical restriction in C++, after all). I'm worried that handling it here will get some cases wrong, such as perhaps:

```
struct A {
  union { int x, y; };
  int z;
};
A a = { .x = 123, 456 }; // should be rejected, but I think this patch might accept
```

... and it might also get template instantiation cases wrong for a case like:

```
struct Q { Q(); };
struct A { Q x, y; };
template<typename T> void f() {
  A a = {.y = Q()};
}
void g() { f<int>(); }
```

... where we might possibly end up passing an `InitListExpr` representing `{Q(), .y = Q()}` into `InitListChecker`.

I think the only C++20 restriction that it makes sense to check in `InitListChecker` is that the field names are in the right order; everything else should be checked earlier. This would also match the semantics of overload resolution, for which the "fields are in the right order" check is deferred until after a function is selected, whereas all the other checks are performed eagerly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59754