Funnel absl usage through base/absl/

[Lei Zhang]

A small bit of friction that I have with
third_party/abseil-cpp/absl/types/optional.h is that it is rather long
and not easy to type out like base/optional.h. Whenever I need to use
absl::optional, I have to find another file that uses it and
copy+paste it in. I mentioned this in the middle of a discussion on
another CL [1] and both avi@ and danakj@ suggested adding base/absl/
headers to forward to third_party/abseil-cpp/absl. avi@ further added
that we can "use forwarding includes in base/absl to indicate which
parts of absl are ok to use in Chromium, and ban all other includes
except for that file." I also like this idea, so I'm sending it to
cxx@ for discussion with a wider audience.

[1] https://crrev.com/c/2910828

[Victor Vasiliev]

As far as I remember, the path recommended by the Abseil upstream is absl/types/optional.h, which is what some of the Chromium dependencies (WebRTC and QUICHE) already use.  Is there any good reason to not just use that?

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

[dan...@chromium.org]

On Fri, Jun 18, 2021 at 12:17 PM Victor Vasiliev <vas...@chromium.org> wrote:

As far as I remember, the path recommended by the Abseil upstream is absl/types/optional.h, which is what some of the Chromium dependencies (WebRTC and QUICHE) already use.  Is there any good reason to not just use that?

I think it'd actually be more confusing/ambiguous to do that, and probably just not compile. Due to how non-first-party code includes abseil, we actually have third_party/abseil-cpp in the include path so that code can use absl/types/optional.h. That means first-party code could too, but should not. We explicitly require the full path for clarity on all includes.

If we had absl/ at the top level to produce the same path, we'd have 2 includes with the same path available to the compiler.

I think base/absl/ is a good middle-ground, where we can think of abseil as being exposed though/as-if part of base.

[Jeremy Roman]

I don't really understand what's particularly compelling about this case as a reason to deviate from our usual style of using the full path from the src/ directory. In upstream Google code long include paths are routine, and even within Chromium we have frequently-used headers with includes like:

#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"

which is considerably longer than the Abseil path at issue here.

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

[Jeremy Roman]

Of additional note: all of upstream uses "third_party/absl/...", which is the actual path. We could presumably modify our roll procedure to remove that one directory component (abseil-cpp/) if it is a significant barrier.

[dan...@chromium.org]

On Mon, Jun 21, 2021 at 2:51 PM Jeremy Roman <jbr...@chromium.org> wrote:

Of additional note: all of upstream uses "third_party/absl/...", which is the actual path. We could presumably modify our roll procedure to remove that one directory component (abseil-cpp/) if it is a significant barrier.

I think this will not be compatible with our DEPS infrastructure, and would require some heavy work, much more time/effort than some forwarding headers in base. So I would not personally prefer that path.

 

On Mon, Jun 21, 2021 at 2:47 PM Jeremy Roman <jbr...@chromium.org> wrote:

I don't really understand what's particularly compelling about this case as a reason to deviate from our usual style of using the full path from the src/ directory. In upstream Google code long include paths are routine, and even within Chromium we have frequently-used headers with includes like:

#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"

This also requires code search and copy paste to find the path to anything, if you're not intimately familiar with the structure. But these headers are less common than core "base"-like things such as optional.

[Nico Weber]

+1 to keeping things as they are. One of the points of absl is that upstream deps like webrtc can use it too, and having a forwarding header or a different path makes it harder to move files between upstream and downstream.

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

[dan...@chromium.org]

On Tue, Jun 22, 2021 at 12:24 PM Nico Weber <tha...@chromium.org> wrote:

+1 to keeping things as they are. One of the points of absl is that upstream deps like webrtc can use it too, and having a forwarding header or a different path makes it harder to move files between upstream and downstream.

Isn't the include path in downstream already different (not in third_party/abseil-cpp?) https://source.chromium.org/search?q=f:webrtc%20f:third_party%20include.*absl%2F&ss=chromium%2Fchromium%2Fsrc

[Tal Pressman]

My 2c: As someone who generally relies on VS Code to add missing includes, I would prefer to keep the "long" include path, since automated tools tend to pick the actual headers where things are declared and not the forwarding headers.

[Mirko Bonadei]

On Tuesday, June 22, 2021 at 6:30:12 PM UTC+2 danakj wrote:

On Tue, Jun 22, 2021 at 12:24 PM Nico Weber <tha...@chromium.org> wrote:

+1 to keeping things as they are. One of the points of absl is that upstream deps like webrtc can use it too, and having a forwarding header or a different path makes it harder to move files between upstream and downstream.

Isn't the include path in downstream already different (not in third_party/abseil-cpp?) https://source.chromium.org/search?q=f:webrtc%20f:third_party%20include.*absl%2F&ss=chromium%2Fchromium%2Fsrc

Yes, WebRTC uses relative paths "absl/..." because it needs to be flexible on there absl is on different repositories, so we cannot use the full path from //.

I tend to prefer the current situation as well because it creates less confusion, absl is only in one place, there is one header, etc..