absl::variant and narrowing conversions

[Florian Leimgruber]

Hi!

std::variant's constructor disallows narrowing conversions, but it seems like the version of absl::variant used in Chromium doesn't - at least not in all cases. In particular, I just noticed that in the following snipped, absl prefers bool over std::string:

absl::variant<bool, std::string> v = "value";
// absl::holds_alternative<bool>(v) is true

Is this a known bug?

In the std::variant migration bug (b/40242126), incompatibilities around narrowing conversions are mentioned, but they seem focused on compilation errors introduced by switching to std::variant. In the case above, the code compiles fine with either variant, but the behavior is different.

Thanks,

Florian

[David Benjamin]

Yes, this is known. absl::variant is a few iterations on std::variant behind. See https://github.com/llvm/llvm-project/pull/73121#issuecomment-1823408375 for the history here. Given that most of Google has since switched to std::variant, I doubt there'll be much effort spent on absl::variant at this point. The right fix here is to finish the migration to std::variant, which is blocked on resolving some bad codegen in libc++'s std::variant.

This actually was already resolved by a Clang compilation pass in https://chromium-review.googlesource.com/c/chromium/src/+/5469234, but there were some concerns about it not yet being enabled by default. (It's not enabled by default due to some issues compiling Flang specifically, which doesn't matter for Chromium.) I have not had time to push that past the finish line and probably won't for a bit. @Nico Weber, perhaps you, or someone on your team would have the cycles to resolve this? We would need to either accept enabling the compilation pass, or implement the same optimization in libc++ directly and then hope that does not also cause issues for Flang.

As for the Chromium issues being fixed in the migration, it's not true that it has only been focused on compilation errors. Rather, it's simply that compilation errors are the most common way that this surfaces. There are corner cases where both compile, but absl::variant selects the wrong one. I think there was one instance of that in the codebase, though I forget if I fixed it or if the code was removed on its own. Either way, tests should fail in that case, and then we'll catch it that way.

--
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/2eca5084-9f48-405a-b457-1967bdbc49ffn%40chromium.org.

[Florian Leimgruber]

Awesome, thanks for confirming and providing all this context!

Florian Leimgruber

Software Engineer

fleim...@google.com

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde. 

     

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.