Page MenuHomePhabricator

[Sema] Add c++2a designated initializer warnings
Needs ReviewPublic

Authored by hintonda on Mar 24 2019, 4:48 PM.

Details

Summary

Basic designated initializers are allowed in c++2a, so only
issue warnings for the extended ones that aren't allowed:

  • out of order
  • nested
  • arrays
  • mixed

Event Timeline

hintonda created this revision.Mar 24 2019, 4:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2019, 4:48 PM
hintonda edited the summary of this revision. (Show Details)Mar 24 2019, 4:49 PM
hintonda retitled this revision from [Sema] Add c++2a designator initializer warnings to [Sema] Add c++2a designated initializer warnings.Mar 24 2019, 4:53 PM
hintonda edited the summary of this revision. (Show Details)
xbolva00 added inline comments.Mar 24 2019, 5:07 PM
clang/lib/Sema/SemaInit.cpp
2002

typo

hintonda updated this revision to Diff 192049.Mar 24 2019, 5:35 PM
  • Fix variable typo/spellings, and add missing flag assignment and additional test to catch it.
Rakete1111 added inline comments.
clang/lib/Sema/SemaInit.cpp
2017

Can Field ever be FieldEnd at this point?

2044

!VerifyOnly and braces please.

2070

Same here. It should be !VerifyOnly. Also braces for the if statement.

3097–3107

Missing dot.

3099

Braces.

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
30

Those warnings are misleading, since C++20 does have designated initializers; they just don't support some stuff that C99 does. It would be better IMO if you could separate them. As in, the above should give you: out-of-order designated initializers are a C99 feature or something like that.

hintonda marked 2 inline comments as done.Mar 25 2019, 2:44 PM

Thanks for the review.

clang/lib/Sema/SemaInit.cpp
2017

Yes. If the last trip through the loop was a designated initializer, and it happened to be the last field in the RecordDecl, Field will be equal FieldEnd. At this point, the loop will exit below unless there's another designated initializer that's out of order.

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
30

I think that would be a good idea as well, but wanted to get advise first.

lebedev.ri added inline comments.
clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
30

As in, the above should give you: out-of-order designated initializers are a C99 feature or something like that.

I suppose also the question is, whether to error-out, or support them as an extension?

hintonda marked an inline comment as done.Mar 25 2019, 2:57 PM
hintonda added inline comments.
clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
30

Although most of them seem fine, the nested ones can be problematic. Please see https://reviews.llvm.org/D17407 for a proposal on how to fix them.

Rakete1111 added inline comments.Mar 25 2019, 3:03 PM
clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
30

I suppose also the question is, whether to error-out, or support them as an extension?

Yes that's true. gcc doesn't support them at all in C++, and it seems like we accept it as well, but only for C classes (constructors make clang crash).

But making it an error now breaks backwards compatibility. So I think the best solution is to accept it for now, as an extension.

hintonda marked an inline comment as done.Mar 25 2019, 5:26 PM
hintonda added inline comments.
clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
30

Btw, several cpp tests use nested designated initializers. Here's a quick list I got by temporarily making it an error:

CodeGenCXX/mangle-exprs.cpp
SemaCXX/cxx2a-initializer-aggregates.cpp
SemaCXX/decltype.cpp
SemaTemplate/instantiate-init.cpp

I believe there are a several out of order ones as well, but those might have been unintentional.

hintonda marked an inline comment as done.Mar 25 2019, 5:44 PM
hintonda added inline comments.
clang/lib/Sema/SemaInit.cpp
2044

Will commit this change as soon as check-clang finishes, but not sure I grok the VerityOnly/!VerifyOnly criteria. Could you help me out?

hintonda updated this revision to Diff 192232.Mar 25 2019, 5:56 PM
  • Address comments.
hintonda updated this revision to Diff 192233.Mar 25 2019, 5:59 PM
  • Missed one set of braces.
Rakete1111 added inline comments.Mar 26 2019, 2:35 AM
clang/lib/Sema/SemaInit.cpp
2044

VerifyOnly is used if you only want to verify and not actually generate any diagnostics or actually do anything that would commit to that change. So only generate the warning if !VerifyOnly.

ping, and should we add new warning messages as suggested by @Rakete1111 ?

rsmith added inline comments.Apr 15 2019, 6:50 PM
clang/lib/Sema/SemaInit.cpp
2017

Please use an actual type rather than auto here and below.

2046

This should use a distinct diagnostic that says what part is an extension.

3101

Likewise here, we should say what specifically is an extension.

hintonda updated this revision to Diff 195610.Apr 17 2019, 11:44 AM
  • Removed auto and added specific warnings.
hintonda updated this revision to Diff 195611.Apr 17 2019, 11:48 AM
  • Fix typo.

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.

clang/include/clang/Basic/DiagnosticSemaKinds.td
158

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".

159–160

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.

clang/lib/Sema/SemaInit.cpp
2069–2072

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.

hintonda marked an inline comment as done.Apr 21 2019, 1:14 PM
hintonda added inline comments.
clang/lib/Sema/SemaInit.cpp
2069–2072

Will work on moving these to the parser.

Btw, the first one is diagnosed correctly, but doesn't complain about the second. Not sure I grok the problem there either since Q has a default ctor.

hintonda marked an inline comment as done.Apr 22 2019, 6:59 AM
hintonda added inline comments.
clang/lib/Sema/SemaInit.cpp
2069–2072

Woke up this morning and realized what you meant about the union. I'll take care of it in the next patch.

thanks again...

This looks like exactly what we need for my project. We're using Clang and Designated Initializers but would like to make sure that we use those in C++20 compatible manner. Is this blocked on something? Any way I can help?

This looks like exactly what we need for my project. We're using Clang and Designated Initializers but would like to make sure that we use those in C++20 compatible manner. Is this blocked on something? Any way I can help?

The basic logic is here modulo @rsmith's comments. I just haven't had time to address them, and unfortunately, it might be another few weeks before I can get back to it.

If you'd like to work on it and finish it up, that would be great.

xbolva00 resigned from this revision.Jul 19 2019, 12:40 PM