C++17 proposal: Ban std::in_place family

[Avi Drissman]

The std::in_place family are disambiguation tags for std::optional, std::variant, and std::any. We ban all three of those.

We allow the absl:: versions of optional and variant, and those two require the absl:: version of the in_place family.

We therefore should ban the std:: version of the in_place family, and for those instances where our own code uses a base:: version of in_place tags (e.g. RefCountedData) we should migrate from the base:: version to the absl:: version and only have the absl:: version used everywhere in Chromium code.

Avi

[dan...@chromium.org]

+1

--
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/CACWgwAZX98PYeAr-OdJRC9Fn5XoqzvB8n_GRahKsLwgSELPY3g%40mail.gmail.com.

[Avi Drissman]

I'm having an issue implementing this.

For optional, we do

#include "third_party/abseil-cpp/absl/types/optional.h"

and for in_place I'm trying to do 

#include "third_party/abseil-cpp/absl/utility/utility.h"

The problem is that I'm getting a compile error:

In file included from ../../base/allocator/partition_allocator/starscan/pcscan_scheduling.cc:15:
In file included from ../../base/bind.h:13:
In file included from ../../base/bind_internal.h:17:
In file included from ../../base/callback_internal.h:15:
In file included from ../../base/memory/ref_counted.h:24:
../../third_party/abseil-cpp/absl/utility/utility.h:48:10: fatal error: 'absl/base/config.h' file not found
#include "absl/base/config.h"
         ^~~~~~~~~~~~~~~~~~~~

But the optional header has exactly the same include and that works just fine.

Thoughts?

[Peter Kasting]

On the run, but this sounds like someone somewhere doesn't have the right deps (or public_deps) on Abseil in their .gn.  They need that to pull in the config that adds the Abseil base dir as an include dir.

PK

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

[K. Moon]

+1; I suspect including optional.h from the same file doesn't work, either.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFB%3Dqxr7n3cDd0Mh15iCZafqw%2BinJg81yvH%3D5FN8%3D0b8YA%40mail.gmail.com.

[Honglin Yu]

The cause may be that for optional.h, ""//third_party/abseil-cpp/absl/base:config" is a public_deps (code) whereas for utility.h, it is a private one (code). Maybe try to make the utility.h deps as public.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-4HVzAmFW25oqwxR%3D18smVtW0LMU24BUNsdCOkZZdAWuw%40mail.gmail.com.

[Avi Drissman]

Thank you! This should help me be able to finish up the cleanup of template_util.

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

[Avi Drissman]

I'm afraid that isn't enough to get it to work. Is there someone here familiar with the absl BUILD situation?

[Peter Kasting]

On Thu, Apr 28, 2022 at 7:36 AM Avi Drissman <a...@google.com> wrote:

I'm afraid that isn't enough to get it to work. Is there someone here familiar with the absl BUILD situation?

I'm likely the closest outside of the absl OWNERS.

PK

[Avi Drissman]

I was going to upload a snapshot, but I’m getting tons of include issues so I have to poke that hole too.

I’m wondering if this ban is worth it, and if we should just go and use the std::in_place versions for our own stuff and the absl:: one for the absl:: stuff, and pay the price of having two in_places to gain simplicity of not having to have more absl around than we need.

[Peter Kasting]

On Thu, Apr 28, 2022 at 9:05 AM Avi Drissman <a...@google.com> wrote:

I’m wondering if this ban is worth it, and if we should just go and use the std::in_place versions for our own stuff and the absl:: one for the absl:: stuff, and pay the price of having two in_places to gain simplicity of not having to have more absl around than we need.

I think you're just hitting deps issues that we'd hit otherwise anyway as we use absl more, so it's worth trying to figure out.

PK 

[dan...@chromium.org]

That's my intuition as well FWIW

 

PK 

[Avi Drissman]

My current version of this is https://chromium-review.googlesource.com/c/chromium/src/+/3614476 and you can see the errors. Any thoughts as to what I'm missing?

Thanks.

[Peter Kasting]

On Thu, Apr 28, 2022 at 11:50 AM Avi Drissman <a...@google.com> wrote:

My current version of this is https://chromium-review.googlesource.com/c/chromium/src/+/3614476 and you can see the errors. Any thoughts as to what I'm missing?

It doesn't look like the partition_alloc target actually has a direct deps on //base?  Maybe adding that would help.

Though it's sort of weird, I thought from the existence of partition_alloc_base/ that partition_alloc was supposed to have its own standalone trimmed-down "base" and not use base/...

PK

[dan...@chromium.org]

PA is in the process of dropping its use of base, so I can imagine maybe this was is broken along the way.

 

PK

[Avi Drissman]

PA is directly using bind.h, callback.h, and ref_counted.h which all wind to

In file included from ../../base/callback.h:15:

In file included from ../../base/bind.h:13:
In file included from ../../base/bind_internal.h:17:
In file included from ../../base/callback_internal.h:15:
In file included from ../../base/memory/ref_counted.h:24:

and it seems like the use in ref_counted is what's killing it.

I don't want to add a dep if that's something they're trying to get away from, but keeping a separate implementation of in_place is silly.

Right now we have two impls of in_place: base::in_place and absl::in_place. If PA prevents us from moving to just using absl::in_place everywhere, is going to two impls but one of them being a std:: one better?

Avi

[Peter Kasting]

On Thu, Apr 28, 2022 at 12:30 PM Avi Drissman <a...@google.com> wrote:

PA is directly using bind.h, callback.h, and ref_counted.h

...so it should depend on //base.  They can remove that if and when they actually remove their dependence on //base, but until then, the .gn is lying and broken.

PK

[Avi Drissman]

[0/1/0] Regenerating ninja files

ERROR Dependency cycle:
  //base:base ->
  //base/allocator/partition_allocator:partition_alloc ->
  //base:base

So base depends on PA, and I don't think in a "allow_circular_includes_from" way.

[Peter Kasting]

On Thu, Apr 28, 2022 at 12:43 PM Avi Drissman <a...@google.com> wrote:

[0/1/0] Regenerating ninja files

ERROR Dependency cycle:
  //base:base ->
  //base/allocator/partition_allocator:partition_alloc ->
  //base:base

So base depends on PA, and I don't think in a "allow_circular_includes_from" way.

LOL.  So PA _can't_ depend on //base, yet it does.  PA is broken.  I don't immediately know how to fix this other than "Will someone who owns PA please fix :("

PK 

[Avi Drissman]

I mean, they kinda are working on it. https://crbug.com/1151236

In the meanwhile, do we want to leave the status quo alone, and keep using both base::in_place and absl::in_place, or switch the base:: usage to std:: and clear out our copy?

[Peter Kasting]

On Thu, Apr 28, 2022 at 12:49 PM Avi Drissman <a...@google.com> wrote:

I mean, they kinda are working on it. https://crbug.com/1151236

In the meanwhile, do we want to leave the status quo alone, and keep using both base::in_place and absl::in_place, or switch the base:: usage to std:: and clear out our copy?

I'd just leave the status quo alone, sigh.

PK 

[Avi Drissman]

Dana pointed out that I could add a deps to PA and make it work, and that seems to have unblocked things.

Meanwhile, there are a lot of places in the code that include base/ headers in their own headers, and I'm chasing down deps and turning them into public_deps. We already have a big issue with undeclared public_deps causing build flakiness, and this seems like yet another way it's biting us.

But hopefully I can get this posted soon.

[dan...@chromium.org]

On Fri, Apr 29, 2022 at 2:19 PM Avi Drissman <a...@google.com> wrote:

Dana pointed out that I could add a deps to PA and make it work, and that seems to have unblocked things.

Meanwhile, there are a lot of places in the code that include base/ headers in their own headers, and I'm chasing down deps and turning them into public_deps. We already have a big issue with undeclared public_deps causing build flakiness, and this seems like yet another way it's biting us.

I wonder: GN check verifies that you depend on what you include. It can't really do that transitively. But could it check that you public_depend on what you publicly include?

[Avi Drissman]

That would be nice.

Random example that I'm fixing right now:

https://source.chromium.org/chromium/chromium/src/+/main:ios/public/provider/chrome/browser/discover_feed/discover_feed_api.h;l=17?q=unique_ptr%3CDiscoverFeedService%3E%20CreateDiscoverFeedService&ss=chromium

This .h file includes stuff from ios/chrome/browser/discover_feed/. The build file only has a deps, not a public deps, even though it's a header file that includes other headers.

Is that something we can catch?

[dan...@chromium.org]

On Fri, Apr 29, 2022 at 4:19 PM Avi Drissman <a...@google.com> wrote:

That would be nice.

Random example that I'm fixing right now:

https://source.chromium.org/chromium/chromium/src/+/main:ios/public/provider/chrome/browser/discover_feed/discover_feed_api.h;l=17?q=unique_ptr%3CDiscoverFeedService%3E%20CreateDiscoverFeedService&ss=chromium

This .h file includes stuff from ios/chrome/browser/discover_feed/. The build file only has a deps, not a public deps, even though it's a header file that includes other headers.

Is that something we can catch?

Here's what's currently checked! https://source.chromium.org/gn/gn/+/master:src/gn/header_checker_unittest.cc

[Sylvain Defresne]

On Fri, Apr 29, 2022 at 9:05 PM <dan...@chromium.org> wrote:

On Fri, Apr 29, 2022 at 2:19 PM Avi Drissman <a...@google.com> wrote:

Dana pointed out that I could add a deps to PA and make it work, and that seems to have unblocked things.

Meanwhile, there are a lot of places in the code that include base/ headers in their own headers, and I'm chasing down deps and turning them into public_deps. We already have a big issue with undeclared public_deps causing build flakiness, and this seems like yet another way it's biting us.

I wonder: GN check verifies that you depend on what you include. It can't really do that transitively. But could it check that you public_depend on what you publicly include?

This is a problem I've reported to gn (https://bugs.chromium.org/p/gn/issues/detail?id=287). I agree that `gn check` should check that includes from public headers should be present in public_deps and not deps. I've got the bug assigned to myself because I reported it, but I probably won't have time to make any change immediately, so if anyone wants to work on this, feel free to do so and steal the bug.

 

But hopefully I can get this posted soon.

On Thu, Apr 28, 2022 at 3:53 PM Peter Kasting <pkas...@google.com> wrote:

On Thu, Apr 28, 2022 at 12:49 PM Avi Drissman <a...@google.com> wrote:

I mean, they kinda are working on it. https://crbug.com/1151236

In the meanwhile, do we want to leave the status quo alone, and keep using both base::in_place and absl::in_place, or switch the base:: usage to std:: and clear out our copy?

I'd just leave the status quo alone, sigh.

PK 

Cheers,

-- Sylvain

 

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/CAHtyhaSQWe%3D_c5XrRP6Hg3ruEfA3KR%3DvYpMO_zbVEyyVA1BgWA%40mail.gmail.com.

[dan...@chromium.org]

On Mon, May 2, 2022 at 5:37 AM Sylvain Defresne <sdef...@chromium.org> wrote:

On Fri, Apr 29, 2022 at 9:05 PM <dan...@chromium.org> wrote:

On Fri, Apr 29, 2022 at 2:19 PM Avi Drissman <a...@google.com> wrote:

Dana pointed out that I could add a deps to PA and make it work, and that seems to have unblocked things.

Meanwhile, there are a lot of places in the code that include base/ headers in their own headers, and I'm chasing down deps and turning them into public_deps. We already have a big issue with undeclared public_deps causing build flakiness, and this seems like yet another way it's biting us.

I wonder: GN check verifies that you depend on what you include. It can't really do that transitively. But could it check that you public_depend on what you publicly include?

This is a problem I've reported to gn (https://bugs.chromium.org/p/gn/issues/detail?id=287). I agree that `gn check` should check that includes from public headers should be present in public_deps and not deps. I've got the bug assigned to myself because I reported it, but I probably won't have time to make any change immediately, so if anyone wants to work on this, feel free to do so and steal the bug.

I recently landed a bunch of changes in GN (in different code but anyhow...) and would be happy to help mentor anyone who wants to pick this up as best that I am able. Thanks for filing this bug Sylvain.