C++17 feature proposal: Structured bindings

[Victor Costan]

Hi folks,

Structured bindings is my favorite C++17 feature. https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++11.md#Structured-bindings-tbd

Therefore, I'm nominating this as (one of) the first C++17 features to be allowed in Chromium.

Looking forward to your thoughts!

    Victor

[Honglin Yu]

+1, it can help get rid of ".first" and ".second" (which are vague and verbose) when doing range-based loops through maps.

--
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/CAP_mGKojW-XjiLBdavuh4iKx56S92-oqzvQB-nssa6u_-SWwBw%40mail.gmail.com.

[Peter Kasting]

+1

PK

[Roland Bock]

+1

On Fri, Dec 24, 2021 at 3:42 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

+1

PK

--
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/CAAHOzFDRhOrsD0Z170XaQxLFsQ%3DPBesJqP3_xM%3DLYGz5U8K_Ng%40mail.gmail.com.

[Daniel Cheng]

Are there any subtleties worth noting? For example, structured binding uses 'auto' but always creates an alias, whereas 'auto' elsewhere follows the normal rules for template argument deduction.

Daniel

On Fri, 24 Dec 2021 at 07:09, 'Roland Bock' via cxx <c...@chromium.org> wrote:

+1

On Fri, Dec 24, 2021 at 3:42 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

+1

PK

--
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/CAAHOzFDRhOrsD0Z170XaQxLFsQ%3DPBesJqP3_xM%3DLYGz5U8K_Ng%40mail.gmail.com.

--
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/CAHq1K7QxJacN4%2Bt3nan5Rx%3D%2B7ZsVjye7H4oqbxGDgJ9%3DGH77VA%40mail.gmail.com.

[Avi Drissman]

It's not really an "alias" and it's different for tuple types. https://jguegant.github.io/blogs/tech/structured-bindings.html has great info on how to think of it.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKqZamjG7e5RnSxCk8hNOnMX5eu78cC%2B-%3DEhvL74i9wscQ%40mail.gmail.com.

[Victor Costan]

CL proposing this change:  https://crrev.com/c/3356325

I'd appreciate reviews!

    Victor

[Daniel Cheng]

Well, I wasn't quite sure what to call it. I guess my point is that they're not really normal variables (for example, they do not work with lambda capture in C++17).

Daniel

[Victor Costan]

https://crrev.com/c/3356325 has 1 LGTM. I'm planning to submit on Monday, unless I hear otherwise by then.

Thank you!

    Victor

[Peter Kasting]

I'd give things a few more days, just because there are so many holidays around now that some people may not have had a chance to weigh in.

PK

[Honglin Yu]

As Daniel mentioned, it seems there is indeed inconsistent behavior between compilers regarding capturing the bindings in lambdas: e.g. it is allowed in gcc but not in clang (even adding -std=c++20). Maybe we should at least be careful with such a controversial situation? 

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

[Daniel Cheng]

The clang behavior is standards-compliant and is what we'd expect in Chrome. I think it's still worth a mention in our documentation as a kind of weird edge case. I believe there's an internal C++ TotW that discusses this, but that particular one has not been made public yet.

I believe http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1091r3.html fixes this for C++20 though.

Daniel

2021年12月27日(月) 12:01 Honglin Yu <hong...@google.com>:

[dan...@chromium.org]

+1 to allowing this.

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

[Nico Weber]

I have two questions about the CL:

1. It replaces

    FontEnumerationStatus status;
    base::ReadOnlySharedMemoryRegion region;
    std::tie(status, region) = manager_sync_->EnumerateLocalFonts();

with

    auto [status, region] = manager_sync_->EnumerateLocalFonts();

How does this interact with the guidance of spelling out the type unless it's obvious (https://google.github.io/styleguide/cppguide.html#Type_deduction)?

2. In the same function, a few lines further down, it replaces

    std::tie(status, region) = manager_sync_->EnumerateLocalFonts();

with

    auto [status2, region2] = manager_sync_->EnumerateLocalFonts();

(and then it replaces status and region with status2 and region2 in the following code.)

Is introducing two new variables with a trailing `2` preferable over just calling tie() here?

I don't have strong opinions about what the answers to these questions should be, but it might be nice to think about this and then have some guidance summarizing community consensus in the guide.

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

[Peter Kasting]

On Fri, Jan 7, 2022 at 11:20 AM Nico Weber <tha...@chromium.org> wrote:

I have two questions about the CL:

1. It replaces

    FontEnumerationStatus status;
    base::ReadOnlySharedMemoryRegion region;
    std::tie(status, region) = manager_sync_->EnumerateLocalFonts();

with

    auto [status, region] = manager_sync_->EnumerateLocalFonts();

How does this interact with the guidance of spelling out the type unless it's obvious (https://google.github.io/styleguide/cppguide.html#Type_deduction)?

That guidance does go on to give specific guidance for structured bindings that say that they're often a net readability win, so my reading of the upstream guide is that it thinks usage of structured bindings is OK even though they sacrifice type names, and the precise usage at each location is "judgement call".  Here, for example, as a reviewer I would judge that the variable names are already giving much the same information as the type names, so this change would probably be OK.  That said, one could argue for a change to "auto [font_enumeration_status, shared_memory_region] = ..." or the like.

2. In the same function, a few lines further down, it replaces

    std::tie(status, region) = manager_sync_->EnumerateLocalFonts();

with

    auto [status2, region2] = manager_sync_->EnumerateLocalFonts();

(and then it replaces status and region with status2 and region2 in the following code.)

Is introducing two new variables with a trailing `2` preferable over just calling tie() here?

I don't love "2"s, but I do prefer SSA-style usage of variables where possible over "reuse a temp for a similar purpose".  It makes lifetimes clearer.  (Even better: individual scope blocks with {}, that use the same temporary variable names inside each one.)  That seems like total individual preference, though.  I'm not sure we need to give any guidance about it?

PK

[Roland Bock]

I also read the guidance this way. The structured bindings code is

 - more concise

 - allows for more const

This improvement in readability should balance the absence of explicit types.

[Nico Weber]

On Mon, Jan 10, 2022 at 11:55 AM Peter Kasting <pkas...@google.com> wrote:

On Fri, Jan 7, 2022 at 11:20 AM Nico Weber <tha...@chromium.org> wrote:

I have two questions about the CL:

1. It replaces

    FontEnumerationStatus status;
    base::ReadOnlySharedMemoryRegion region;
    std::tie(status, region) = manager_sync_->EnumerateLocalFonts();

with

    auto [status, region] = manager_sync_->EnumerateLocalFonts();

How does this interact with the guidance of spelling out the type unless it's obvious (https://google.github.io/styleguide/cppguide.html#Type_deduction)?

That guidance does go on to give specific guidance for structured bindings that say that they're often a net readability win, so my reading of the upstream guide is that it thinks usage of structured bindings is OK even though they sacrifice type names, and the precise usage at each location is "judgement call".

That's not my reading of the style guide (but maybe I'm reading it wrong). As far as I can tell, the structure is (represented as a nested list):

  Type Deduction

    Definition: Here are contexts in which type deduction can or must be used

      List of examples that includes, among other things, structured bindings. The examples describe how the features work but don't give style advice on them.

    Decision: "Use type deduction only to make the code clearer or safer, and do not use it merely to avoid the inconvenience of writing an explicit type. [...] These principles apply to all forms of type deduction, but the details vary, as described in the following sections."

As I read this, the section gives an overview over the various types, and then the unifying guidance "generally, only use if type is clear".

That is, the Google C++ style guide as I read it doesn't say how to reconcile the general advice "only omit type when it's clear" with "structured bindings force you to omit the type".

[Peter Kasting]

On Mon, Jan 10, 2022 at 10:00 AM Nico Weber <tha...@chromium.org> wrote:

On Mon, Jan 10, 2022 at 11:55 AM Peter Kasting <pkas...@google.com> wrote:

On Fri, Jan 7, 2022 at 11:20 AM Nico Weber <tha...@chromium.org> wrote:

I have two questions about the CL:

1. It replaces

    FontEnumerationStatus status;
    base::ReadOnlySharedMemoryRegion region;
    std::tie(status, region) = manager_sync_->EnumerateLocalFonts();

with

    auto [status, region] = manager_sync_->EnumerateLocalFonts();

How does this interact with the guidance of spelling out the type unless it's obvious (https://google.github.io/styleguide/cppguide.html#Type_deduction)?

That guidance does go on to give specific guidance for structured bindings that say that they're often a net readability win, so my reading of the upstream guide is that it thinks usage of structured bindings is OK even though they sacrifice type names, and the precise usage at each location is "judgement call".

That's not my reading of the style guide (but maybe I'm reading it wrong). As far as I can tell, the structure is (represented as a nested list):

  Type Deduction

    Definition: Here are contexts in which type deduction can or must be used

      List of examples that includes, among other things, structured bindings. The examples describe how the features work but don't give style advice on them.

    Decision: "Use type deduction only to make the code clearer or safer, and do not use it merely to avoid the inconvenience of writing an explicit type. [...] These principles apply to all forms of type deduction, but the details vary, as described in the following sections."

I think this omits the important followon bit:

"Unlike other forms of type deduction, structured bindings can actually give the reader additional information, by giving meaningful names to the elements of a larger object. This means that a structured binding declaration may provide a net readability improvement over an explicit type, even in cases where auto would not. Structured bindings are especially beneficial when the object is a pair or tuple..."

To me, that amounts to a caveat that partially (but not completely) speaks to your concern.

PK

[dan...@chromium.org]

FWIW I generally let auto get used in a lot more places in tests than in prod code as it helps to avoid testing the types involved which are not the point of the test.

If this was production code I would not like the use of auto here, as it's not _more_ clear by using auto, and it's not an iterator in a loop. I think the correct solution here for production is to use a struct with named fields, not a pair, as the return type.

[Victor Costan]

I updated the CL based on the feedback posted in the code review.

Nico, would you be OK with concluding that the C++ style guide allows the changes in the CL?

Thank you,

    Victor

[Nico Weber]

Yes, fine with me.

Personally, I think it'd be nice if the blurb included a "This feature forces omitting the type name, so only use it where knowledge of the concrete type isn't important, consistent with the general guidance for auto".

[Avi Drissman]

I'm seeing a dangerous situation with structured bindings that I'm failing to get a warning on. Filed as https://crbug.com/1290897 but noting here for people's thoughts.

Something that structured bindings can be good for is iterating over a std::map. I reviewed https://crrev.com/c/3413750 which turned

for (const auto& pr : element_to_data_lookup_)

into

for (const auto [element, data] : element_to_data_lookup_)

which is missing the &. Now, this should warn, and a sample I threw into Compiler Explorer did warn. If I took the sample and dumped it into Chromium, I got a warning, but the actual CL doesn't warn.

As noted again, I filed this, but folks should be careful in migrating this.

Avi

[Avi Drissman]

Actually, never mind. I mis-read what map was being looped on in the CL.