This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by hankadusikova on Apr 7 2021, 11:39 AM.

Details

Reviewers
jfb
rsmith
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

hankadusikova requested review of this revision.Apr 7 2021, 11:39 AM
hankadusikova created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2021, 11:39 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
miscco added a subscriber: miscco.Apr 7 2021, 11:49 AM
miscco added inline comments.
clang/lib/Sema/SemaInit.cpp
1013–1023

There is a typo here aggreagates -> aggregates

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 marked an inline comment as done.Apr 7 2021, 12:02 PM

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.

hankadusikova edited the summary of this revision. (Show Details)Apr 7 2021, 12:25 PM
jfb accepted this revision.Apr 7 2021, 1:45 PM

A few nits, but this is good otherwise!

clang/lib/Sema/SemaInit.cpp
1013–1023

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

1025–1040

Same here.

clang/test/SemaCXX/aggregate-initialization.cpp
183

Haha I like the name! 🧅😭

This revision is now accepted and ready to land.Apr 7 2021, 1:45 PM
hankadusikova marked 3 inline comments as done.Apr 7 2021, 2:01 PM

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

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 accepted this revision.Apr 12 2021, 12:53 PM

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

1025
hankadusikova marked 3 inline comments as done.

changes suggested by @rsmith

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?

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