Remove warning "suggest braces" for aggregate initialization of an empty class with an aggregate base class.

[hankadusikova]

Summary

I recently ran into issues with aggregates and inheritance, I'm using it for creating a type-safe library where most of the types are build over "tagged" std::array. After bit of cleaning and enabling -Wall -Wextra -pedantic I noticed clang only in my pipeline gives me warning. After a bit of focusing on it I found it's not helpful, and contemplate disabling the warning all together. After a discussion with other library authors I found it's bothering more people and decided to fix it.

Removes this warning:

template<typename T, int N> struct StdArray {
    T contents[N];
  };

template<typename T, int N> struct AggregateAndEmpty : StdArray<T,N> { };

AggregateAndEmpty<int, 3> p = {1, 2, 3}; // <-- warning here about omitted braces

This warning starts to be prominent when you have multiple levels of inheritance for strong typing with aggregates:

template<typename T, int N> struct StdArray {
    T contents[N];
};

template <typename T, int N> struct JustABaseClass : StdArray<T, N> {};
template <typename T, int N> struct OnionBaseClass : JustABaseClass<T, N> {};

OnionBaseClass<int,3> i = {1,2,3}; // instead {{{1,2,3}}}

This is my first patch for clang, so I hopefully didn't overlook something, but all tests are green.

[miscco]

clang/lib/Sema/SemaInit.cpp

1013–1034

There is a typo here aggreagates -> aggregates

[hankadusikova]

Fix typo.

[lebedev.ri]

This patch says what it does, but not why it does what it does.
Did the pattern become idiomatic so that we now want to not warn?

[hankadusikova]

In D100057#2674820, @lebedev.ri wrote:

This patch says what it does, but not why it does what it does.
Did the pattern become idiomatic so that we now want to not warn?

Yes, using inheritance for strong typing with aggregates is quite common pattern. I checked GCC / EDG / MSVC and neither of them is emitting warning in this case.

[jfb]

A few nits, but this is good otherwise!

clang/lib/Sema/SemaInit.cpp

1013–1034

"Allow eliding brace initialization".

Also, period at the end of the sentence.

1018

Multiple empty bases isn't a common thing, right? I don't think so, but would rather check.

1036

Same here.

clang/test/SemaCXX/aggregate-initialization.cpp

183

Haha I like the name! 🧅😭

[hankadusikova]

In D100057#2675085, @jfb wrote:

A few nits, but this is good otherwise!

Having an aggregate with multiple (empty) bases initialised with nonempty {} is a hard error, hence the 1.

https://compiler-explorer.com/z/hKqxWjo3G

clang/lib/Sema/SemaInit.cpp

1018

Having an aggregate with multiple (empty) bases initialised with {} is a hard error, hence the 1.

https://compiler-explorer.com/z/hKqxWjo3G

[ldionne]

FWIW, I think this is acceptable, but I think even better would be to make it clear in the Standard that this is acceptable. Having spoken with a few folks about this, it doesn't appear to be specified very clearly.

[rsmith]

Thanks, I think this makes sense as a generalization of the existing "idiomatic aggregate initialization" rule. Some suggested cleanups but otherwise LGTM.

clang/lib/Sema/SemaInit.cpp

1001

I wonder if it'd be clearer to replace the one base and no fields check and the one field and no bases check with something like:

// Brace elision is idiomatic if we're initializing the only direct subobject of
// an aggregate of record type.
if (Entity.getKind() == InitializedEntity::EK_Base ||
    Entity.getKind() == InitializedEntity::EK_Member) {
  auto *ParentRD = Entity.getParent()->getType()->getAsRecordDecl();
  unsigned Subobjects = std::distance(ParentRD->field_begin(), ParentRD->field_end());
  if (auto *CXXRD = dyn_cast<CXXRecordDecl>(ParentRD))
    Subobjects += CXXRD->getNumBases();
  return Subobjects == 1;
}

This'd be linear in the number of fields instead of constant-time, but I suspect that doesn't matter in practice. Aggregate initialization is linear in the number of fields regardless.

(No strong preference here; take this or leave it.)

1017–1021

Some minor simplifications. (We can cast rather than dyn_casting here because we're initializing a base class, so the parent *must* be a C++ class type.)

1036

[hankadusikova]

changes suggested by @rsmith

[aaron.ballman]

Do you need someone to commit this on your behalf? If so, what email address and name would you like me to use for commit attribution?

[aaron.ballman]

Thank you for the patch! I've commit on your behalf in 64c24f493e5f4637ee193f10f469cdd2695b4ba6.

clang/lib/Sema/SemaInit.cpp

int InitListChecker::numStructUnionElements(QualType DeclType) {
 
   if (structDecl->isUnion())
     return std::min(InitializableMembers, 1);
   return InitializableMembers - structDecl->hasFlexibleArrayMember();
 }
 
 /// Determine whether Entity is an entity for which it is idiomatic to elide
 /// the braces in aggregate initialization.
 static bool isIdiomaticBraceElisionEntity(const InitializedEntity &Entity) {
   // Recursive initialization of the one and only field within an aggregate
   // class is considered idiomatic. This case arises in particular for
   // initialization of std::array, where the C++ standard suggests the idiom of
   //
   //   std::array<T, N> arr = {1, 2, 3};
   //
   // (where std::array is an aggregate struct containing a single array field.
 
-  // FIXME: Should aggregate initialization of a struct with a single
-  // base class and no members also suppress the warning?
-  if (Entity.getKind() != InitializedEntity::EK_Member || !Entity.getParent())
+  if (!Entity.getParent())
     return false;
 
+  // Allows elide brace initialization for aggregates with empty base.
+  if (Entity.getKind() == InitializedEntity::EK_Base) {
     auto *ParentRD =
         Entity.getParent()->getType()->castAs<RecordType>()->getDecl();
-  if (CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(ParentRD))
-    if (CXXRD->getNumBases())
-      return false;
+    CXXRecordDecl *CXXRD = cast<CXXRecordDecl>(ParentRD);
+    return CXXRD->getNumBases() == 1 && CXXRD->field_empty();
+  }
 
+  // Allow brace elision if the only subobject is a field.
+  if (Entity.getKind() == InitializedEntity::EK_Member) {
+    auto *ParentRD =
+        Entity.getParent()->getType()->castAs<RecordType>()->getDecl();
+    if (CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(ParentRD)) {
+      if (CXXRD->getNumBases()) {
+        return false;
+      }
+    }
     auto FieldIt = ParentRD->field_begin();
     assert(FieldIt != ParentRD->field_end() &&
            "no fields but have initializer for member?");
     return ++FieldIt == ParentRD->field_end();
   }
 
+  return false;
+}
+
 /// Check whether the range of the initializer \p ParentIList from element
 /// \p Index onwards can be used to initialize an object of type \p T. Update
 /// \p Index to indicate how many elements of the list were consumed.
 ///
 /// This also fills in \p StructuredList, from element \p StructuredIndex
 /// onwards, with the fully-braced, desugared form of the initialization.
 void InitListChecker::CheckImplicitInitList(const InitializedEntity &Entity,
                                             InitListExpr *ParentIList,

clang/test/SemaCXX/aggregate-initialization.cpp

#pragma clang diagnostic warning "-Wmissing-braces"
   ArrayAndSomethingElse<int, 3> y = {1, 2, 3}; // expected-warning {{suggest braces}}
 
 #if __cplusplus >= 201703L
   template<typename T, int N> struct ArrayAndBaseClass : StdArray<int, 3> {
     T contents[N];
   };
   ArrayAndBaseClass<int, 3> z = {1, 2, 3}; // expected-warning {{suggest braces}}
 
-  // It's not clear whether we should be warning in this case. If this
-  // pattern becomes idiomatic, it would be reasonable to suppress the
-  // warning here too.
+  // This pattern is used for tagged aggregates and must not warn
   template<typename T, int N> struct JustABaseClass : StdArray<T, N> {};
-  JustABaseClass<int, 3> w = {1, 2, 3}; // expected-warning {{suggest braces}}
+  JustABaseClass<int, 3> w = {1, 2, 3};
+  // but this should be also ok
+  JustABaseClass<int, 3> v = {{1, 2, 3}};
+
+  template <typename T, int N> struct OnionBaseClass : JustABaseClass<T, N> {};
+  OnionBaseClass<int, 3> u = {1, 2, 3};
+  OnionBaseClass<int, 3> t = {{{1, 2, 3}}};
+
+  struct EmptyBase {};
+
+  template <typename T, int N> struct AggregateAndEmpty : StdArray<T, N>, EmptyBase {};
+  AggregateAndEmpty<int, 3> p = {1, 2, 3}; // expected-warning {{suggest braces}}
 #endif
 
 #pragma clang diagnostic pop
 }
 
 namespace HugeArraysUseArrayFiller {
   // All we're checking here is that initialization completes in a reasonable
   // amount of time.