PSA: absl::optional is now std::optional

[David Benjamin]

Hi all,

As of https://crrev.com/1204351, absl::optional is now a type alias for std::optional. Likewise, absl::nullopt is now an alias for std::nullopt. Please feel free to start writing std spellings instead in new code.

A handy cheat sheet:

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

absl::optional => std::optional
absl::nullopt => std::nullopt
absl::in_place => std::in_place
absl::in_place_t => std::in_place_t

David

[Anton Bikineev]

Shall we consider a rewrite? I guess a simple sed-like rewrite would do the trick.

--
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/CAF8qwaAe28FmirLG6%3D3HRg%2BzaqHdp9WiMt2A%3Dcn5OyYNwcbfPg%40mail.gmail.com.

[David Benjamin]

Probably! I don't think I'll have the cycles to drive a giant rewrite, but I'd definitely be in support of it.

[Peter Boström]

I think IWYU is the non-trivial thing, iirc no tool fixes that for us automatically.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF8qwaAWNs24oR%3DO1%3Dq%2BTPbSO7_xMuyZTMF9Cu2Yc6MR%3DLQKeQ%40mail.gmail.com.

[Peter Boström]

(unless our tools are better than when I left on parental leave)

[Anton Bikineev]

I think IWYU is the non-trivial thing, iirc no tool fixes that for us automatically.

I converted base::Optional to absl::optional a while ago using a custom clang-tidy rewriter which also fixed the includes. I can share the rewriter sources if somebody is willing to tackle this (I myself lack the cycles unfortunately).

[Daniel Cheng]

We don't have a functioning IWYU, but generally, these sorts of things can be (relatively) easily fixed up by script if there's enough of them. We already have an add_header.py script that automates the work of inserting the header, so it's just a matter of mapping compile errors to the corresponding header (or proactively fixing them).

Daniel

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

[Alexei Svitkine]

For splitting, you can use `git cl split` after making the changes to all files locally and it will automatically break it up into separate CLs and send to appropriate owners.

On Mon, Nov 6, 2023 at 1:20 PM 'Arthur Sonzogni' via cxx <c...@chromium.org> wrote:

Would the rewrite be more complex than a search and replace?

Suggested script:

#!/bin/bash
 
function replace {
  echo "Replacing $1 by $2"
  git grep -l "$1" | cut -f1 -d: | grep "\.h$" | xargs sed -i "s/$1/$2/g"
  git grep -l "$1" | cut -f1 -d: | grep "\.cc$" | xargs sed -i "s/$1/$2/g"
}
 
 
replace "absl::optional" "std::optional"
replace "absl::nullopt" "std::nullopt"
replace "absl::in_place" "std::in_place"
replace "absl::in_place_t" "std::in_place_t"
replace "\"third_party\/abseil-cpp\/absl\/types\/optional.h\"" "<optional>"
git cl format

This creates a 20k file change, modifying 103k lines:
https://chromium-review.googlesource.com/c/chromium/src/+/5003622

If this had been a large change associated with "//base", I would have sent the patch to a top-level base OWNER with Owners-override.
Here, this is an "abseil" vs "system" header, so I would probably first ask for a LSC change approval.
If you are interested and there are no particular difficulties, I can take a look tomorrow and ask to execute this, probably split into smaller patches to avoid constant merge conflicts.

Arthur @arthursonzogni

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKpLgScy%3DFNyprnb2AFv2TB3JQr1Hd8pMtchangesd%2BOmOysWg4_A%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/CAAzos5HTKwm%3D%3D_17x%2BA3bUMEg7Vf6NAjMc4OXEimhoEBgL85rA%40mail.gmail.com.

[Dana Jansens]

👍

On Mon, Nov 6, 2023 at 1:20 PM 'Arthur Sonzogni' via cxx <c...@chromium.org> wrote:

Would the rewrite be more complex than a search and replace?

Suggested script:

#!/bin/bash
 
function replace {
  echo "Replacing $1 by $2"
  git grep -l "$1" | cut -f1 -d: | grep "\.h$" | xargs sed -i "s/$1/$2/g"
  git grep -l "$1" | cut -f1 -d: | grep "\.cc$" | xargs sed -i "s/$1/$2/g"
}
 
 
replace "absl::optional" "std::optional"
replace "absl::nullopt" "std::nullopt"
replace "absl::in_place" "std::in_place"
replace "absl::in_place_t" "std::in_place_t"
replace "\"third_party\/abseil-cpp\/absl\/types\/optional.h\"" "<optional>"
git cl format

This creates a 20k file change, modifying 103k lines:
https://chromium-review.googlesource.com/c/chromium/src/+/5003622

If this had been a large change associated with "//base", I would have sent the patch to a top-level base OWNER with Owners-override.
Here, this is an "abseil" vs "system" header, so I would probably first ask for a LSC change approval.
If you are interested and there are no particular difficulties, I can take a look tomorrow and ask to execute this, probably split into smaller patches to avoid constant merge conflicts.

Arthur @arthursonzogni

On Mon, Nov 6, 2023 at 6:44 PM Daniel Cheng <dch...@chromium.org> wrote:

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKpLgScy%3DFNyprnb2AFv2TB3JQr1Hd8pMtchangesd%2BOmOysWg4_A%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/CAAzos5HTKwm%3D%3D_17x%2BA3bUMEg7Vf6NAjMc4OXEimhoEBgL85rA%40mail.gmail.com.

[Peter Boström]

Thank you Arthur! I wouldn't think that a LSC is required here, just splitting this into reasonable OWNERS coverage would make sense though (and yield fewer merge conflicts).

[Sylvain Defresne]

IIRC this script will miss all .mm files. If you go this way, don't forget to also fix them.

As a followup steps, it would probably also be necessary to remove deps or public_deps on third_party/abseil-cpp:absl target that are no longer necessary (which could be more difficult than the string substitution).

-- Sylvain

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

[Lei Zhang]

The add_header.py script also doesn't understand <windows.h> likes to
be first, and tries to combine it into the list of C-headers. It does
a good job overall, but one should check its work.

> To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJauWCGA%3D%2BVo4Ak__APCcGox8btxDp8AgUVTL65X%3DMZC_BGvow%40mail.gmail.com.

[Greg Thompson]

Hi Lei.

While we used to have a policy of always putting <windows.h> above all other headers, ISTR that the modern thought is to put it where we would normally put it, and only bump it to the top if necessary. I don't recall when this happened, but my brain is telling me that's the situation...

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

[Arthur Sonzogni]

The rewrite is now 100% completed! 🎉🎉🎉

This was about 150k lines change over 20k files. 

absl::optional is now banned via a PRESUBMIT.py rule.

From the chromium repository, the following directories have been excluded.

• third_party/abseil-cpp/*
• third_party/googletest/*
• third_party/leveldatabase/*
• third_party/libaddressinput/
• third_party/liburlpattern/*
• third_party/lzma_sdk/*
• third_party/maldoca/*
• third_party/mediapipe/*
• third_party/shell-encryption/
• third_party/tflite_support/*
• third_party/webrtc_overrides/*
• third_party/zxcvbn-cpp/*

On Tuesday, November 14, 2023 at 7:26:46 PM UTC+1 Sylvain Defresne wrote:

FTR, I've converted all of //ios to use std::optional instead of absl::optional.

-- Sylvain

[danakj]

Thanks Arthur, you put a lot of time and effort into this migration. And thanks to all the reviewers for the many CLs to let this get done.

[K. Moon]

Congrats and thank you! Would be interested to hear more about how the experience of making this change went, and what could be improved for next time.

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

[Arthur Sonzogni]

Congrats and thank you! Would be interested to hear more about how the experience of making this change went, and what could be improved for next time.

Good idea! Here is a a public doc about it.

Arthur @arthursonzogni

[Peter Kasting]

On Tue, Feb 27, 2024 at 8:00 AM 'Arthur Sonzogni' via cxx <c...@chromium.org> wrote:

Congrats and thank you! Would be interested to hear more about how the experience of making this change went, and what could be improved for next time.

Good idea! Here is a a public doc about it.

Thanks for writing this. Some of my own takeaways:

* We should try to switch clang-format to regroup include blocks

* ...but when we do we should also modify the include categories to put windows.h alone

* IWYU fixes would help

* All LSCs like this should use OO+1 aggressively to get things landed immediately

* Predator is kinda dumb and harasses people with unrelated crashes when they touch stuff. No immediate fix in sight tho :(

PK