181050 2017-12-20 11:34:51 -0800 Cleanup: Dereference value of optional directly instead of using checked value in WebCore::subdivide() 2018-01-08 09:18:43 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Local Build All All RESOLVED FIXED InRadar P2 Normal --- 1 dbates dbates darin jfbastien simon.fraser webkit-bug-importer oldest_to_newest 1383766 0 dbates 2017-12-20 11:34:51 -0800 Cleanup: Deference value of optional directly instead of using checked value in WebCore::subdivide(). 1383768 1 329940 dbates 2017-12-20 11:41:25 -0800 Created attachment 329940 Patch 1383776 2 329940 simon.fraser 2017-12-20 12:00:50 -0800 Comment on attachment 329940 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329940&action=review > Source/WebCore/ChangeLog:3 > + Cleanup: Deference value of optional directly instead of using checked value in WebCore::subdivide() Dereference 1383777 3 dbates 2017-12-20 12:05:03 -0800 Committed r226195: <https://trac.webkit.org/changeset/226195> 1383779 4 webkit-bug-importer 2017-12-20 12:06:14 -0800 <rdar://problem/36161451> 1386600 5 darin 2018-01-07 22:20:57 -0800 This does not seem right. Not harmful, but not helpful either. I thought that std::optional::operator* and std::optional::value are supposed to have identical semantics. The choice of which one to use should be about clarity, not runtime checking cost. Looking at our Optional.h header, in fact it seems that currently operator* has the assertion and value does not. Which seems quite peculiar. Not sure why. JF? 1386757 6 jfbastien 2018-01-08 09:18:43 -0800 (In reply to Darin Adler from comment #5) > This does not seem right. Not harmful, but not helpful either. > > I thought that std::optional::operator* and std::optional::value are > supposed to have identical semantics. The choice of which one to use should > be about clarity, not runtime checking cost. > > Looking at our Optional.h header, in fact it seems that currently operator* > has the assertion and value does not. Which seems quite peculiar. Not sure > why. > > JF? They don't have the same semantics when the optional doesn't contain a value. operator*() says: Requires: *this contains a value. Therefore using it when the optional contains no value is UB. A valid implementation could throw or abort in these cases, or just return whatever random thing the memory contained. Whereas value() has no requires clause, and instead says: Effects: Equivalent to: return bool(*this) ? *val : throw bad_optional_access(); Without exception support, the right thing to do in value() is to abort. So I agree with Dan that it's sufficient to use operator* when you've proven that a value is indeed contained. I'm not sure it's more efficient: if we've proven it then presumably the compiler can eliminate the value check: since it's the same thing, that check should just go away, I'd look at the assembly. If the compiler doesn't do it I'd say it's a bug, or it hasn't been proven sufficiently :-) Maybe our implementation of optional should be changed to abort always on bad usage? Another pattern that's nice is unwrapping the value and then just using that. Not sure that works here? Maybe value_or() would be appropriate (though seems not either...). 329940 2017-12-20 11:41:25 -0800 2017-12-20 12:00:50 -0800 Patch bug-181050-20171220114125.patch text/plain 2830 dbates U3VidmVyc2lvbiBSZXZpc2lvbjogMjI2MTkzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZjE2Yjc1YzNmMjExNmM1 ZjQzZTU4NDBkMGEwNzFhMDdhMDA3NDQwMy4uNzRiNjAzZTkwNWY5MmEwMmVmZjdlOTZjMzdjMTll NGUzOTQ5YTg2ZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE3LTEyLTIwICBEYW5p ZWwgQmF0ZXMgIDxkYWJhdGVzQGFwcGxlLmNvbT4KKworICAgICAgICBDbGVhbnVwOiBEZWZlcmVu Y2UgdmFsdWUgb2Ygb3B0aW9uYWwgZGlyZWN0bHkgaW5zdGVhZCBvZiB1c2luZyBjaGVja2VkIHZh bHVlIGluIFdlYkNvcmU6OnN1YmRpdmlkZSgpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xODEwNTAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBJdCBpcyBzdWZmaWNpZW50IGFuZCBtb3JlIGVmZmljaWVudCB0 byBkZXJlZmVyZW5jZSBhIHN0ZDo6b3B0aW9uYWwgZGlyZWN0bHkgd2hlbiB3ZSBrbm93IHRoYXQg aXQgaGFzCisgICAgICAgIGEgdmFsdWUgYXMgb3Bwb3NlZCB0byB1c2luZyB0aGUgY2hlY2tlZCBk ZWZlcmVuY2UgbWVtYmVyIGZ1bmN0aW9uIHN0ZDo6b3B0aW9uYWw8Pjo6dmFsdWUoKS4KKworICAg ICAgICAqIHJlbmRlcmluZy9NYXJrZXJTdWJyYW5nZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpz dWJkaXZpZGUpOgorCiAyMDE3LTEyLTIwICBZb3Vlbm4gRmFibGV0ICA8eW91ZW5uQGFwcGxlLmNv bT4KIAogICAgICAgICBMYXlvdXRUZXN0IGltcG9ydGVkL3czYy93ZWItcGxhdGZvcm0tdGVzdHMv c2VydmljZS13b3JrZXJzL2NhY2hlLXN0b3JhZ2Uvc2VydmljZXdvcmtlci9jYWNoZS1tYXRjaC5o dHRwcy5odG1sIGlzIGEgZmxha3kgZmFpbHVyZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL01hcmtlclN1YnJhbmdlLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9N YXJrZXJTdWJyYW5nZS5jcHAKaW5kZXggY2NjNjMyZDkyZDRkNTQ4MzFhZGIzYmI1MjhmYTk2Y2Rh ZTE2Yjc3YS4uOWVjN2Q4YWJiOWM5NDdlNjM1MTUyYzYzYTU2YTBjZTRkYjMwNjQ0MSAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL01hcmtlclN1YnJhbmdlLmNwcAorKysgYi9T b3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvTWFya2VyU3VicmFuZ2UuY3BwCkBAIC03NiwxMiArNzYs MTIgQEAgVmVjdG9yPE1hcmtlclN1YnJhbmdlPiBzdWJkaXZpZGUoY29uc3QgVmVjdG9yPE1hcmtl clN1YnJhbmdlPiYgc3VicmFuZ2VzLCBPdmVybGEKICAgICAgICAgICAgICAgICBpZiAoZnJvbnRt b3N0KSB7CiAgICAgICAgICAgICAgICAgICAgIGlmIChvdmVybGFwU3RyYXRlZ3kgPT0gT3Zlcmxh cFN0cmF0ZWd5OjpGcm9udG1vc3RXaXRoTG9uZ2VzdEVmZmVjdGl2ZVJhbmdlICYmICFyZXN1bHQu aXNFbXB0eSgpKSB7CiAgICAgICAgICAgICAgICAgICAgICAgICBhdXRvJiBwcmV2aW91cyA9IHJl c3VsdC5sYXN0KCk7Ci0gICAgICAgICAgICAgICAgICAgICAgICBpZiAocHJldmlvdXMuZW5kT2Zm c2V0ID09IG9mZnNldFNvRmFyICYmIHByZXZpb3VzLnR5cGUgPT0gb2Zmc2V0c1tmcm9udG1vc3Qu dmFsdWUoKV0uc3VicmFuZ2UtPnR5cGUgJiYgcHJldmlvdXMubWFya2VyID09IG9mZnNldHNbZnJv bnRtb3N0LnZhbHVlKCldLnN1YnJhbmdlLT5tYXJrZXIpCisgICAgICAgICAgICAgICAgICAgICAg ICBpZiAocHJldmlvdXMuZW5kT2Zmc2V0ID09IG9mZnNldFNvRmFyICYmIHByZXZpb3VzLnR5cGUg PT0gb2Zmc2V0c1sqZnJvbnRtb3N0XS5zdWJyYW5nZS0+dHlwZSAmJiBwcmV2aW91cy5tYXJrZXIg PT0gb2Zmc2V0c1sqZnJvbnRtb3N0XS5zdWJyYW5nZS0+bWFya2VyKQogICAgICAgICAgICAgICAg ICAgICAgICAgICAgIHByZXZpb3VzLmVuZE9mZnNldCA9IG9mZnNldHNbaV0udmFsdWU7CiAgICAg ICAgICAgICAgICAgICAgICAgICBlbHNlCi0gICAgICAgICAgICAgICAgICAgICAgICAgICAgcmVz dWx0LmFwcGVuZCh7IG9mZnNldFNvRmFyLCBvZmZzZXRzW2ldLnZhbHVlLCBvZmZzZXRzW2Zyb250 bW9zdC52YWx1ZSgpXS5zdWJyYW5nZS0+dHlwZSwgb2Zmc2V0c1tmcm9udG1vc3QudmFsdWUoKV0u c3VicmFuZ2UtPm1hcmtlciB9KTsKKyAgICAgICAgICAgICAgICAgICAgICAgICAgICByZXN1bHQu YXBwZW5kKHsgb2Zmc2V0U29GYXIsIG9mZnNldHNbaV0udmFsdWUsIG9mZnNldHNbKmZyb250bW9z dF0uc3VicmFuZ2UtPnR5cGUsIG9mZnNldHNbKmZyb250bW9zdF0uc3VicmFuZ2UtPm1hcmtlciB9 KTsKICAgICAgICAgICAgICAgICAgICAgfSBlbHNlCi0gICAgICAgICAgICAgICAgICAgICAgICBy ZXN1bHQuYXBwZW5kKHsgb2Zmc2V0U29GYXIsIG9mZnNldHNbaV0udmFsdWUsIG9mZnNldHNbZnJv bnRtb3N0LnZhbHVlKCldLnN1YnJhbmdlLT50eXBlLCBvZmZzZXRzW2Zyb250bW9zdC52YWx1ZSgp XS5zdWJyYW5nZS0+bWFya2VyIH0pOworICAgICAgICAgICAgICAgICAgICAgICAgcmVzdWx0LmFw cGVuZCh7IG9mZnNldFNvRmFyLCBvZmZzZXRzW2ldLnZhbHVlLCBvZmZzZXRzWypmcm9udG1vc3Rd LnN1YnJhbmdlLT50eXBlLCBvZmZzZXRzWypmcm9udG1vc3RdLnN1YnJhbmdlLT5tYXJrZXIgfSk7 CiAgICAgICAgICAgICAgICAgfQogICAgICAgICAgICAgfSBlbHNlIHsKICAgICAgICAgICAgICAg ICBmb3IgKHVuc2lnbmVkIGogPSAwOyBqIDwgaTsgKytqKSB7Cg==