Sadness in not being able to use aggregate initialization

[Avi Drissman]

I <3 being able to use aggregate initialization, the C++20 feature that we explicitly allow now. I'm trying to use it to clean up structs, and in particular content::DropData::Metadata which has all these CreateFor* calls that just shove values into member variables. I'd like to replace:

  if (drop_data.html) {
    metadata.push_back(DropData::Metadata::CreateForMimeType(
        DropData::Kind::STRING, base::ASCIIToUTF16(ui::kMimeTypeHTML)));
  }

with

  if (drop_data.html) {
    metadata.push_back(
        DropData::Metadata{.kind = DropData::Kind::STRING,
                           .mime_type = base::ASCIIToUTF16(ui::kMimeTypeHTML)});
  }

But that struct is just too big and lives in a header, so it needs to have an explicit out-of-line constructor/destructor/copy constructor. Which, even if defaulted, is a user-defined constructor, which means that aggregate initialization isn't allowed.

I vaguely recall discussions about whether that was still a useful plugin check. I submit this as evidence of its interaction with new language features.

Avi

[Peter Kasting]

On Mon, Dec 19, 2022 at 4:33 PM 'Avi Drissman' via cxx <c...@chromium.org> wrote:

I vaguely recall discussions about whether that was still a useful plugin check. I submit this as evidence of its interaction with new language features.

Yes; since we were having trouble moving forward with the full-fledged "drop the plugin check" route, one compromise I had proposed was "drop the plugin check for aggregates". I don't think dcheng@ collected data specifically around that compromise proposal.

AFAIK this whole space is still stuck in "nobody seems to want to own driving this forward by ensuring we don't actually blow out the binary size".

PK

[Daniel Cheng]

It's been a known issue for a long time that this plugin check is directly incompatible with aggregate initialization.

I did some measurements awhile back using generated Mojo structs as an imperfect proxy, as well as some tests with things that had std::string fields and std::vector<std::string> fields.

The tests with fields of various types demonstrated mixed results; however, the Mojo experiments definitely demonstrated binary size regressions.

If we want to relax/remove this check, does it make sense to have some guidelines / heuristics for when we should still require explicit ctors rather than allowing them to be defaulted? Even in this specific example, I'm not convinced that aggregate initialization is better: it seems like a factory method/constructor that takes a base::StringPiece and does any necessary internal conversions would be a better fit? e.g. something like DropData::Metadata::CreateStringKind(ui::kMimeTypeHTML). (of course, this does not address the more general problem)

Daniel

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFD-zeaFyuRnP5Fwd3Mt5ePrL8Bkpo35GmNh7ggm5U4OKQ%40mail.gmail.com.

[David Benjamin]

Perhaps we could enlist some compiler folks to help out? In principle, we're just giving the compiler more information. The compiler is free to not inline stuff. It just sounds like we don't like its current size trade-offs.

I share Peter's frustration with the aggregate initializers. Also just generally having a lot of boilerplate to defining a new type in our codebase.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKpB3BxmbU5Xd26R1O8Y1Q1LPDBpM7Ke_jgNij9Y5Cgnsg%40mail.gmail.com.

[Martin Bidlingmaier]

> It just sounds like we don't like its current size trade-offs.

Are we sure that we don't like clang's trade-off? Yes, inlining results in increased binary size, but perhaps we get enough performance gains from it so as to make it worthwhile anyway. Does somebody have numbers?

Even if we want to keep the current trade-off, where copy constructors are presumably inlined only rarely (what about thin-lto though?), we could perhaps change our clang plugin so that it marks calls to implicit copy constructors with the equivalent of "__attribute__((noinline))", but otherwise allow such calls. This would still result in a definition of a copy constructor in each translation unit that uses it, but the linker tries to deduplicate functions defined in several units. So we might get almost the same binary, but without having to explicitly declare and define copy constructors.

[Martin Bidlingmaier]

For the particular issue of aggregate initialization, we could simply modify our clang plugin that prevents implicit copy constructors so that it ignores aggregates. This would allow us to declare aggregates in a header but still use aggregate initialization syntax. It would also be OK per Google C++ style guide [1]:

> These declarations/deletions can be omitted only if they are obvious:

> - If the class has no private section, like a struct or an interface-only base class, then the copyability/movability can be determined by the copyability/movability of any public data members.

I'd be happy to upload a CL if we agree that this is something we want.

[1] https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types

[K. Moon]

The Google C++ style guide was written with Google's internal code in mind. That environment deviates from Chromium's situation; pertinent to this discussion, binary size essentially is ignorable. Shipping a few extra gigabytes often costs a negligible amount compared to other engineering and infrastructure costs.

We therefore have a few specific deviations from the Google C++ style guide where it makes sense for Chromium. We can't just wave those away because the Google style guide says something is OK; we need to collect data and make sure the facts have changed.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/7ee2f886-7594-40b4-8389-028778d9cb2en%40chromium.org.