226161 2021-05-23 22:07:11 -0700 typo in `Optional<T&>::value() const` prevents usage of `Optional<const T&>` 2021-05-25 14:50:56 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Local Build Unspecified Unspecified RESOLVED CONFIGURATION CHANGED P2 Normal --- 1 heycam heycam benjamin cdumez cmarcelo darin ews-watchlist mmaxfield ysuzuki oldest_to_newest 1763020 0 heycam 2021-05-23 22:07:11 -0700 . 1763021 1 429502 heycam 2021-05-23 22:12:24 -0700 Created attachment 429502 Patch 1763022 2 429502 mmaxfield 2021-05-23 22:13:20 -0700 Comment on attachment 429502 Patch Please add a test. (TestWTF) 1763025 3 429503 heycam 2021-05-23 22:33:11 -0700 Created attachment 429503 Patch 1763028 4 429503 mmaxfield 2021-05-23 22:41:14 -0700 Comment on attachment 429503 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429503&action=review > Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:253 > + Optional<int&> optional { x }; In all the places where you want to use an optional reference, you should just be using a pointer instead. You shouldn't have hit this problem in the first place. 1763029 5 heycam 2021-05-23 22:50:03 -0700 Using `const T*` works for me. Should the Optional<T&> specialization be removed? There are a few places in the codebase that use Optional<T&>, specifically Optional<JSObject&>. Yusuke, looks like you added the Optional<T&> specialization. Do you think it's really needed, or should we just be using JSObject* in these cases? I thought perhaps this was for completeness when compared to std::optional, but https://en.cppreference.com/w/cpp/utility/optional says: There are no optional references; a program is ill-formed if it instantiates an optional with a reference type. 1763030 6 429503 darin 2021-05-23 22:53:28 -0700 Comment on attachment 429503 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429503&action=review >> Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:253 >> + Optional<int&> optional { x }; > > In all the places where you want to use an optional reference, you should just be using a pointer instead. You shouldn't have hit this problem in the first place. I agree with this comment. As a style choice we encourage continuing to use pointers for optional references. 1763031 7 cdumez 2021-05-23 22:56:43 -0700 (In reply to Cameron McCormack (:heycam) from comment #5) > Using `const T*` works for me. Should the Optional<T&> specialization be > removed? > > There are a few places in the codebase that use Optional<T&>, specifically > Optional<JSObject&>. Yusuke, looks like you added the Optional<T&> > specialization. Do you think it's really needed, or should we just be using > JSObject* in these cases? > > I thought perhaps this was for completeness when compared to std::optional, > but https://en.cppreference.com/w/cpp/utility/optional says: > > There are no optional references; a program is ill-formed if it > instantiates an > optional with a reference type. Indeed, it appears std::optional doesn't support this: https://www.fluentcpp.com/2018/10/05/pros-cons-optional-references/ Maybe we should just get rid of the functionality altogether in WTF::Optional? Let's see what Yusuke has to say since he added it. I do agree that a T* makes more sense than an Optional<T&>. 1763032 8 darin 2021-05-23 22:57:01 -0700 (In reply to Cameron McCormack (:heycam) from comment #5) > I thought perhaps this was for completeness when compared to std::optional, > but https://en.cppreference.com/w/cpp/utility/optional says: > > There are no optional references; a program is ill-formed if it > instantiates an > optional with a reference type. Very interesting! I would like our WTF::Optional to match std::optional and I intend to switch us off of it. You can see a brief debate in bug 211674 where Chris Dumez suggests that our different semantic of guaranteeing a WTF::Optional is set to WTF::nullopt when moving from it is a reason to stay with WTF::Optional. I’d like to resolve that issue and dump WTF::Optional entirely as soon as we can. In the mean time, I suggest we minimize differences from std::optional, which points to removing our support for optional references. 1763033 9 cdumez 2021-05-23 23:01:13 -0700 (In reply to Darin Adler from comment #8) > (In reply to Cameron McCormack (:heycam) from comment #5) > > I thought perhaps this was for completeness when compared to std::optional, > > but https://en.cppreference.com/w/cpp/utility/optional says: > > > > There are no optional references; a program is ill-formed if it > > instantiates an > > optional with a reference type. > > Very interesting! > > I would like our WTF::Optional to match std::optional and I intend to switch > us off of it. You can see a brief debate in bug 211674 where Chris Dumez > suggests that our different semantic of guaranteeing a WTF::Optional is set > to WTF::nullopt when moving from it is a reason to stay with WTF::Optional. > I’d like to resolve that issue and dump WTF::Optional entirely as soon as we > can. I think we've been advocating using std::exchange() for a while now. We can probably try and switch to std::optional. Hopefully, use-after-move won't be as much as an issue this time around. > In the mean time, I suggest we minimize differences from std::optional, > which points to removing our support for optional references. +1 1763691 10 darin 2021-05-25 14:50:56 -0700 This is obsolete now, because I removed the contents of Optional.hhttps://bugs.webkit.org/show_bug.cgi?id=226161# 429502 2021-05-23 22:12:24 -0700 2021-05-23 22:33:07 -0700 Patch bug-226161-20210524151223.patch text/plain 1118 heycam U3VidmVyc2lvbiBSZXZpc2lvbjogMjc3OTM4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDljYjg3N2U0ZTU3MzViNGNmZWM4OWUy YjQ4NzI0NThjNGUyMTE5MjQuLmFkZjEyZTFhMTQ3YjAwNDhjYmUzZjY2ODUwZTQ0NjBlYThjOTc5 NTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMjEtMDUtMjMgIENhbWVyb24gTWNDb3JtYWNrICA8 aGV5Y2FtQGFwcGxlLmNvbT4KKworICAgICAgICBGaXggdHlwbyBpbiBPcHRpb25hbDxUJj46OnZh bHVlKCkgY29uc3QuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0yMjYxNjEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBUaGlzIHByZXZlbnRzIHVzYWdlIG9mIE9wdGlvbmFsPGNvbnN0IFQmPi4KKworICAgICAg ICAqIHd0Zi9PcHRpb25hbC5oOgorICAgICAgICAoV1RGOjpPcHRpb25hbDxUOjp2YWx1ZSBjb25z dCk6CisKIDIwMjEtMDUtMjIgIENhbWVyb24gTWNDb3JtYWNrICA8aGV5Y2FtQGFwcGxlLmNvbT4K IAogICAgICAgICBQcmV2ZW50IHNiaXggZ2x5cGhzIGZyb20gYmVpbmcgcmVuZGVyZWQgaW4gdGhl IEdQVSBwcm9jZXNzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi9PcHRpb25hbC5oIGIvU291 cmNlL1dURi93dGYvT3B0aW9uYWwuaAppbmRleCA2N2FiYTExYWYxOTczYTM5NTFmNWMzM2M1MDZk MjA5ZjdhNmI1ZmFhLi4wNDQ2YzM1MmU5ZTY1Y2QxNDk3MjczMGE4ZGU0ZWM4ZTM2NzFlZDFmIDEw MDY0NAotLS0gYS9Tb3VyY2UvV1RGL3d0Zi9PcHRpb25hbC5oCisrKyBiL1NvdXJjZS9XVEYvd3Rm L09wdGlvbmFsLmgKQEAgLTY3Myw3ICs2NzMsNyBAQCBwdWJsaWM6CiAgIH0KIAogICBjb25zdGV4 cHIgVCYgdmFsdWUoKSBjb25zdCB7Ci0gICAgUkVMRUFTRV9BU1NFUlRfVU5ERVJfQ09OU1RFWFBS X0NPTlRFWFQocmVmKCkpOworICAgIFJFTEVBU0VfQVNTRVJUX1VOREVSX0NPTlNURVhQUl9DT05U RVhUKHJlZik7CiAgICAgcmV0dXJuICpyZWY7CiAgIH0KIAo= 429503 2021-05-23 22:33:11 -0700 2021-05-23 22:53:28 -0700 Patch bug-226161-20210524153309.patch text/plain 2844 heycam U3VidmVyc2lvbiBSZXZpc2lvbjogMjc3OTM4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDljYjg3N2U0ZTU3MzViNGNmZWM4OWUy YjQ4NzI0NThjNGUyMTE5MjQuLmFkZjEyZTFhMTQ3YjAwNDhjYmUzZjY2ODUwZTQ0NjBlYThjOTc5 NTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMjEtMDUtMjMgIENhbWVyb24gTWNDb3JtYWNrICA8 aGV5Y2FtQGFwcGxlLmNvbT4KKworICAgICAgICBGaXggdHlwbyBpbiBPcHRpb25hbDxUJj46OnZh bHVlKCkgY29uc3QuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0yMjYxNjEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBUaGlzIHByZXZlbnRzIHVzYWdlIG9mIE9wdGlvbmFsPGNvbnN0IFQmPi4KKworICAgICAg ICAqIHd0Zi9PcHRpb25hbC5oOgorICAgICAgICAoV1RGOjpPcHRpb25hbDxUOjp2YWx1ZSBjb25z dCk6CisKIDIwMjEtMDUtMjIgIENhbWVyb24gTWNDb3JtYWNrICA8aGV5Y2FtQGFwcGxlLmNvbT4K IAogICAgICAgICBQcmV2ZW50IHNiaXggZ2x5cGhzIGZyb20gYmVpbmcgcmVuZGVyZWQgaW4gdGhl IEdQVSBwcm9jZXNzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi9PcHRpb25hbC5oIGIvU291 cmNlL1dURi93dGYvT3B0aW9uYWwuaAppbmRleCA2N2FiYTExYWYxOTczYTM5NTFmNWMzM2M1MDZk MjA5ZjdhNmI1ZmFhLi4wNDQ2YzM1MmU5ZTY1Y2QxNDk3MjczMGE4ZGU0ZWM4ZTM2NzFlZDFmIDEw MDY0NAotLS0gYS9Tb3VyY2UvV1RGL3d0Zi9PcHRpb25hbC5oCisrKyBiL1NvdXJjZS9XVEYvd3Rm L09wdGlvbmFsLmgKQEAgLTY3Myw3ICs2NzMsNyBAQCBwdWJsaWM6CiAgIH0KIAogICBjb25zdGV4 cHIgVCYgdmFsdWUoKSBjb25zdCB7Ci0gICAgUkVMRUFTRV9BU1NFUlRfVU5ERVJfQ09OU1RFWFBS X0NPTlRFWFQocmVmKCkpOworICAgIFJFTEVBU0VfQVNTRVJUX1VOREVSX0NPTlNURVhQUl9DT05U RVhUKHJlZik7CiAgICAgcmV0dXJuICpyZWY7CiAgIH0KIApkaWZmIC0tZ2l0IGEvVG9vbHMvQ2hh bmdlTG9nIGIvVG9vbHMvQ2hhbmdlTG9nCmluZGV4IGVhYjA4M2YxZTA3ZDQ1ZTg4YWRiMTQwMWRi YjRjMjE1YjUzNzg1NGEuLjIwODVmYWFjMzVhZTRkZDI3ODA4MDY1OTVmOGFhNTQ1NmE4YWJlNWUg MTAwNjQ0Ci0tLSBhL1Rvb2xzL0NoYW5nZUxvZworKysgYi9Ub29scy9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAyMS0wNS0yMyAgQ2FtZXJvbiBNY0Nvcm1hY2sgIDxoZXljYW1AYXBwbGUu Y29tPgorCisgICAgICAgIEZpeCB0eXBvIGluIE9wdGlvbmFsPFQmPjo6dmFsdWUoKSBjb25zdC4K KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIyNjE2MQor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogVGVzdFdl YktpdEFQSS9UZXN0cy9XVEYvT3B0aW9uYWwuY3BwOgorICAgICAgICAoVGVzdFdlYktpdEFQSTo6 VEVTVCk6CisKIDIwMjEtMDUtMjIgIENocmlzIER1bWV6ICA8Y2R1bWV6QGFwcGxlLmNvbT4KIAog ICAgICAgICBSZXBsYWNlIExvY2tIb2xkZXIgd2l0aCBMb2NrZXIgaW4gbG9jYWwgdmFyaWFibGVz CmRpZmYgLS1naXQgYS9Ub29scy9UZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9PcHRpb25hbC5jcHAg Yi9Ub29scy9UZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9PcHRpb25hbC5jcHAKaW5kZXggMjcyNjAw NGY4NGU2MDNkMDFiMWFhMGQ2ZjZlZDgwYWU3NjRlZjMwMS4uOWU2OTRkYmYyZTYxM2EwN2M3MTky MTMxOTNmMGQyODg4ZGZkNjBiZCAxMDA2NDQKLS0tIGEvVG9vbHMvVGVzdFdlYktpdEFQSS9UZXN0 cy9XVEYvT3B0aW9uYWwuY3BwCisrKyBiL1Rvb2xzL1Rlc3RXZWJLaXRBUEkvVGVzdHMvV1RGL09w dGlvbmFsLmNwcApAQCAtMjM1LDQgKzIzNSw0MSBAQCBURVNUKFdURl9PcHRpb25hbCwgTW92ZVNl bWFudGljc0Fzc2lnbm1lbnQpCiAgICAgfQogfQogCitURVNUKFdURl9PcHRpb25hbCwgUmVmZXJl bmNlKQoreworICAgIHsKKyAgICAgICAgT3B0aW9uYWw8aW50Jj4gb3B0aW9uYWwgeyB9OworICAg ICAgICBFWFBFQ1RfVFJVRSghb3B0aW9uYWwpOworICAgIH0KKworICAgIHsKKyAgICAgICAgT3B0 aW9uYWw8Y29uc3QgaW50Jj4gb3B0aW9uYWwgeyB9OworICAgICAgICBFWFBFQ1RfVFJVRSghb3B0 aW9uYWwpOworICAgIH0KKworICAgIHsKKyAgICAgICAgaW50IHggPSAxMjM7CisKKyAgICAgICAg T3B0aW9uYWw8aW50Jj4gb3B0aW9uYWwgeyB4IH07CisgICAgICAgIEVYUEVDVF9UUlVFKCEhb3B0 aW9uYWwpOworICAgICAgICBFWFBFQ1RfRVEoMTIzLCBvcHRpb25hbC52YWx1ZSgpKTsKKworICAg ICAgICB4ID0gNDU2OworICAgICAgICBFWFBFQ1RfVFJVRSghIW9wdGlvbmFsKTsKKyAgICAgICAg RVhQRUNUX0VRKDQ1Niwgb3B0aW9uYWwudmFsdWUoKSk7CisgICAgfQorCisgICAgeworICAgICAg ICBpbnQgeCA9IDEyMzsKKworICAgICAgICBPcHRpb25hbDxjb25zdCBpbnQmPiBvcHRpb25hbCB7 IHggfTsKKyAgICAgICAgRVhQRUNUX1RSVUUoISFvcHRpb25hbCk7CisgICAgICAgIEVYUEVDVF9F USgxMjMsIG9wdGlvbmFsLnZhbHVlKCkpOworCisgICAgICAgIHggPSA0NTY7CisgICAgICAgIEVY UEVDVF9UUlVFKCEhb3B0aW9uYWwpOworICAgICAgICBFWFBFQ1RfRVEoNDU2LCBvcHRpb25hbC52 YWx1ZSgpKTsKKyAgICB9Cit9CisKIH0gLy8gbmFtZXNwYWNlIFRlc3RXZWJLaXRBUEkK