Page MenuHomePhabricator

[Sema] Add c++2a designated initializer warnings
ClosedPublic

Authored by rsmith 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

Diff Detail

Repository
rL LLVM

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 ↗(On Diff #192048)

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 ↗(On Diff #192049)

Can Field ever be FieldEnd at this point?

2044 ↗(On Diff #192049)

!VerifyOnly and braces please.

2065 ↗(On Diff #192049)

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

3087 ↗(On Diff #192049)

Missing dot.

3089 ↗(On Diff #192049)

Braces.

clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
30 ↗(On Diff #192049)

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 ↗(On Diff #192049)

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 ↗(On Diff #192049)

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 ↗(On Diff #192049)

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 ↗(On Diff #192049)

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 ↗(On Diff #192049)

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 ↗(On Diff #192049)

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 ↗(On Diff #192049)

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 ↗(On Diff #192049)

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 ↗(On Diff #192233)

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

2046 ↗(On Diff #192233)

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

3096 ↗(On Diff #192233)

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 ↗(On Diff #195611)

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 ↗(On Diff #195611)

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 ↗(On Diff #195611)

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 ↗(On Diff #195611)

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 ↗(On Diff #195611)

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
rsmith commandeered this revision.Aug 29 2019, 4:06 PM
rsmith edited reviewers, added: hintonda; removed: rsmith.

I'm taking this over to finish it off and submit.

I'm taking this over to finish it off and submit.

Thanks Richard...

This revision was not accepted when it landed; it landed in state Needs Review.Aug 30 2019, 3:52 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2019, 3:52 PM
xbolva00 added inline comments.
cfe/trunk/lib/Sema/SemaInit.cpp
3112

warning: variable ‘HasArrayDesignator’ set but not used [-Wunused-but-set-variable]

bool HasArrayDesignator = false;

Hi! We've noticed that for our arm bots, we're getting some flaky builds that sometimes fail with error: array designators are a C99 extension [-Werror,-Wc99-designator] and sometimes don't fail. 2 questions:

  1. I can't see it off the patch immediately, but do you know why for arm specifically we can only get this warning sometimes?
  2. I noticed that for the test/SemaCXX/c99.cpp test, this warning is also diagnosed for the -std=c++17 case. Are C-style designated initializers only invalid in c++20, or are they also invalid in 17?

Thanks.

phosek added a subscriber: phosek.Sep 8 2019, 2:40 PM

Hi! We've noticed that for our arm bots, we're getting some flaky builds that sometimes fail with error: array designators are a C99 extension [-Werror,-Wc99-designator] and sometimes don't fail. 2 questions:

  1. I can't see it off the patch immediately, but do you know why for arm specifically we can only get this warning sometimes?
  2. I noticed that for the test/SemaCXX/c99.cpp test, this warning is also diagnosed for the -std=c++17 case. Are C-style designated initializers only invalid in c++20, or are they also invalid in 17?

    Thanks.

@rsmith ping? We're still hitting this issue in our build. Should this warning be diagnosed even when using -std=c++17?

rsmith added a comment.Sep 8 2019, 6:11 PM

Hi! We've noticed that for our arm bots, we're getting some flaky builds that sometimes fail with error: array designators are a C99 extension [-Werror,-Wc99-designator] and sometimes don't fail. 2 questions:

  1. I can't see it off the patch immediately, but do you know why for arm specifically we can only get this warning sometimes?

That is strange; can you provide me with buildbot links for a pass and a failure?

  1. I noticed that for the test/SemaCXX/c99.cpp test, this warning is also diagnosed for the -std=c++17 case. Are C-style designated initializers only invalid in c++20, or are they also invalid in 17?

C-style designated initializers are invalid in all C++ standards before C++20, and some of the features are still invalid in C++20. We allow those features as an extension, but as of this change we produce a warning by default when that extension is used. We really should have been doing that for years, but it matters a lot more now, because people are going to look at Clang's diagnostics to understand which parts of C designator syntax are valid in C++ and which parts are not.

Another question about this, sorry. Do you know _why_ C++20 is more restrictive than C99 wrt "mixture of designated and non-designated initializers in the same initializer list is a C99 extension"? Is there some interaction with other C++ features that makes the C99 behavior difficult in C++20?

Another question about this, sorry. Do you know _why_ C++20 is more restrictive than C99 wrt "mixture of designated and non-designated initializers in the same initializer list is a C99 extension"? Is there some interaction with other C++ features that makes the C99 behavior difficult in C++20?

The design paper P0329R0 describes this behavior but doesn't give much rationale. According to the minutes and my recollections, this was part of the design as presented and didn't really receive any pushback at any stage of the feature design and discussion. I think generally the feeling was that there was insufficient motivation for adding that level of complexity (much like with the restriction to single-level designators and lack of support for array designators).