Also rename Int32Poisoned to ConstExprPoisoned. The key it takes is actually a uint32_t. So, Int32 is really a misnomer. In addition, the key needs to be a constexpr. So, ConstExprPoisoned is a better name for it.
<rdar://problem/36006884>
Created attachment 329169 [details] proposed patch.
Comment on attachment 329169 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=329169&action=review r=me > Source/WTF/wtf/Poisoned.h:55 > +struct isValidReference : std::integral_constant<bool, !isFunctionPointer<T>::value && !isVoidPointer<T>::value> { }; What's a "valid reference"? What makes it "valid"? > Source/WTF/wtf/Poisoned.h:194 > +constexpr uintptr_t makePoison(uint32_t key) inline?
Comment on attachment 329169 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=329169&action=review >> Source/WTF/wtf/Poisoned.h:55 >> +struct isValidReference : std::integral_constant<bool, !isFunctionPointer<T>::value && !isVoidPointer<T>::value> { }; > > What's a "valid reference"? What makes it "valid"? I'll rename this to isConvertibleToReference, which is what I really meant. >> Source/WTF/wtf/Poisoned.h:194 >> +constexpr uintptr_t makePoison(uint32_t key) > > inline? Thanks. Fixed.
Created attachment 329172 [details] patch for landing + speculative build fixes.
Created attachment 329182 [details] patch for landing + speculative build fixes 2. The build failures on the EWS bots are weird: it fails to specialize the PoisonedImpl template correctly for the Poisoned variant, but does it correctly for the ConstExprPoisoned variant. I cannot reproduce this build failure locally, but the build failure error messages seem to imply that the EWS toolchain is confused with what the type of the poison key should be used in its inferrence. I'm now uploading a patch that takes the linkage of the key out of the file where it's used to see if can kick the EWS toolchain into not inferring the wrong type. Let's see if this makes a difference.
Created attachment 329185 [details] test to see if EWS toolchains support C++14.
Created attachment 329187 [details] test for EWS.
Created attachment 329188 [details] test for EWS.
Created attachment 329189 [details] test for EWS.
Created attachment 329195 [details] test for EWS.
Created attachment 329197 [details] test for EWS.
I've managed to reproduce this build error on godbolt.org. This is what godbolt.org says: gcc 6.3 - Build fails gcc 7.1 - Build fails gcc 7.2 - Build passes clang 3.9.1 - Build fails clang 4.0.0 - Build passes In all cases, I was using -std=c++14. Hence, the issue is with the compilers and not my code. That said, let me see if there's a way I can work around this compiler bug.
Do you have a small repro from godbolt?
Created attachment 329205 [details] patch for landing. I'm going to go with disabling the use of the Poisoned template methods that take a PoisonedImpl<K2, k, T2> argument in the tests that uses these methods with the uintptr_t& key type. The compiler bug does not manifest when the methods are used with a constexpr uintptr_t key type. In practice, we're likely to only assigned values which are poisoned with different keys when using the ConstExprPoisoned variant. Hence, this compiler bug will not be an issue. To make sure we don't break any builds, we only need to #if out the use of these methods in the API tests. Since we don't use these methods for the Poisoned variant in real code, there's nothing more to do there. I'll land this patch if the EWS bots are happy with it.
Surprise surprise. It looks like the old buggy compilers don't like assignment of Poisoned<keyA, DerivedClassPtr> to Poisoned<keyA, BaseClassPtr> either. The only think it seems to support is assignment of Poisoned<keyA, SameClassPtr> to Poisoned<keyA, SameClassPtr>. Again, this does not affect the ConstExprPoisoned variant. I'll exclude these additional cases from the tests as well.
Created attachment 329206 [details] patch for landing.
Created attachment 329207 [details] patch for landing.
Created attachment 329212 [details] patch for landing. Looks like the EWS thinks it's using at least clang 4, but is still failing to build. Let's try filtering ENABLE_MIXED_POISON on clang 5 instead.
Created attachment 329213 [details] patch for landing. Looks like Apple's versioning of clang is different than the open source one. Here's a patch based with a speculative fix based on that versioning difference.
Created attachment 329229 [details] patch for landing.
Attachment 329229 [details] did not pass style-queue: ERROR: Source/WTF/wtf/Poisoned.h:48: Missing space after , [whitespace/comma] [3] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
The bots are green. I'm going to land this.
Thanks for the review. Landed in r225857: <http://trac.webkit.org/r225857>.