Page MenuHomePhabricator

[ADT] Make Optional a literal type.
ClosedPublic

Authored by varungandhi-apple on Aug 21 2020, 8:45 AM.

Details

Summary

This allows returning Optional values from constexpr contexts.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2020, 8:45 AM
varungandhi-apple requested review of this revision.Aug 21 2020, 8:45 AM
fhahn accepted this revision.Aug 26 2020, 12:03 PM
fhahn added reviewers: dblaikie, rjmccall.

This looks reasonable to me, but it might be good to wait a bit so other people can also chime in.

This revision is now accepted and ready to land.Aug 26 2020, 12:03 PM
rjmccall accepted this revision.Aug 26 2020, 12:19 PM

This seems to only allow empty optionals to be constexpr. Should the non-default constructor also be constexpr? (Note that it's okay for a templated constexpr function to instantiate to something that does stuff that's not constexpr, so this wouldn't be a major new restriction on the constructor.)

dblaikie accepted this revision.Aug 26 2020, 12:37 PM
dblaikie added inline comments.
llvm/unittests/ADT/OptionalTest.cpp
20–23

This might be a bit unnecessary, if the test below is adequate? (this seems like a place where testing on the public interface would be sufficient & make it easier if the implementation details are refactored later)

24

is_literal_type is deprecated in C++17 and removed in C++20 - might be better to test the property in practical terms by making some constexpr variables (in test cases, or not)

constexpr Optional<int> o;

etc?

This seems to only allow empty optionals to be constexpr. Should the non-default constructor also be constexpr?

The reason I implemented the minimal thing was that I wasn't sure how far I should go. In a Swift PR, @davezarzycki raised the point about constexpr potentially leading to longer compile times even when not used for compile-time computation. I asked some folks internally at Apple and the response I got was along the lines of, "That's surprising... If that's actually the case, it should be reported as a Clang bug". I haven't been able to dedicate time to benchmarking the impact of constexpr on compile time... Since Optional is a currency type, I wasn't sure if people would object to more methods being marked constexpr. In principle, I don't see any reason for any method on Optional to not be constexpr.

It's an abstractly reasonable change. I wouldn't worry about some sort of conjectured impact on build times in the absence of specific evidence.

This seems to only allow empty optionals to be constexpr. Should the non-default constructor also be constexpr?

The reason I implemented the minimal thing was that I wasn't sure how far I should go. In a Swift PR, @davezarzycki raised the point about constexpr potentially leading to longer compile times even when not used for compile-time computation. I asked some folks internally at Apple and the response I got was along the lines of, "That's surprising... If that's actually the case, it should be reported as a Clang bug". I haven't been able to dedicate time to benchmarking the impact of constexpr on compile time... Since Optional is a currency type, I wasn't sure if people would object to more methods being marked constexpr. In principle, I don't see any reason for any method on Optional to not be constexpr.

That upstream thread doesn't look like it's talking about the impact of constexpr in non-constexpr contexts. Yes, making a variable constexpr when you don't need it to be can have negative compile time consequences - but in Clang I don't believe (again, can be bugs, but it doesn't sound like that swift thread is suggesting the existence of such bugs) a constexpr function called in a non-constexpr context is handled any differently than if it were a non-constexpr function. (GCC is different in this regard and does try to do constexpr things in non-constexpr contexts, though unclear how hard it tries before bailing out to the non-constexpr handling)

  1. Allow many more operations in constexpr.
  2. Update test to reflect use in constexpr instead of relying on implementation details.
varungandhi-apple marked 2 inline comments as done.Aug 26 2020, 4:07 PM
This comment was removed by varungandhi-apple.

I wasn't really asking for massive constexpr-ification, just on the other constructors, but I have no problem with it given that it's done.

This seems to only allow empty optionals to be constexpr. Should the non-default constructor also be constexpr?

The reason I implemented the minimal thing was that I wasn't sure how far I should go. In a Swift PR, @davezarzycki raised the point about constexpr potentially leading to longer compile times even when not used for compile-time computation. I asked some folks internally at Apple and the response I got was along the lines of, "That's surprising... If that's actually the case, it should be reported as a Clang bug". I haven't been able to dedicate time to benchmarking the impact of constexpr on compile time... Since Optional is a currency type, I wasn't sure if people would object to more methods being marked constexpr. In principle, I don't see any reason for any method on Optional to not be constexpr.

That upstream thread doesn't look like it's talking about the impact of constexpr in non-constexpr contexts. Yes, making a variable constexpr when you don't need it to be can have negative compile time consequences - but in Clang I don't believe (again, can be bugs, but it doesn't sound like that swift thread is suggesting the existence of such bugs) a constexpr function called in a non-constexpr context is handled any differently than if it were a non-constexpr function. (GCC is different in this regard and does try to do constexpr things in non-constexpr contexts, though unclear how hard it tries before bailing out to the non-constexpr handling)

Actually, I was concerned about constexpr affecting non-constexpr contexts. Perhaps it was the gcc behavior that this former coworker was worried about (and I wish I remember who told me this so I could ask them but alas this feedback was many years ago). I'm happy to learn that clang isn't trying to be clever about evaluating constexpr in non-constexpr contexts.

(I'm going to wait till at least Monday to land this, in case anyone has objections or would like to suggest changes to the implementation or tests.)

This revision was landed with ongoing or failed builds.Sep 1 2020, 4:14 PM
This revision was automatically updated to reflect the committed changes.