After wtf/Poisoned.h was added there are a number of warnings from Clang when building. Source\WTF\wtf\Poisoned.h(204,62): warning : implicit conversion of nullptr constant to 'bool' [-Wnull-conversion] This should be globally defined since it follows with the style guide.
Created attachment 330875 [details] Patch
Created attachment 330876 [details] Patch
Hm, can you show an example of code that triggers this warning? It sounds like it warns when a nullptr constant should be changed to "false" but I presume you would not be reporting this as a bug unless there were more to it....
Comment on attachment 330876 [details] Patch Attachment 330876 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6011905 New failing tests: http/tests/workers/service/postmessage-after-sw-process-crash.https.html
Created attachment 330881 [details] Archive of layout-test-results from ews105 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
(In reply to Michael Catanzaro from comment #3) > Hm, can you show an example of code that triggers this warning? It sounds > like it warns when a nullptr constant should be changed to "false" but I > presume you would not be reporting this as a bug unless there were more to > it.... Don mentioned https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/Poisoned.h#L204 in IRC. This code looks fine, however I'm not sure if this warning should be really ignored everywhere
I think this warning is possibly slightly valuable and just turning it off is the wrong thing to do unless it’s really hard to silence it. I don’t agree that our coding style requires disabling this warning; it’s true that our coding style contributes slightly to the problem because we are supposed to do things like "ptr ? x : y" rather than "ptr != nullptr ? x : y", but I’d like to help fix the warnings rather than disabling them. Unless the implementation of the warning is particularly poor. I can think of two ways to fix the warning in the particular case of Poisoned::poison. The trick is to overload for nullptr_t. Adding line of code like this almost certainly work: constexpr static PoisonedBits poison(std::nullptr_t) { return 0; } An alternate solution, a little less elegant, assuming that it’s specifically the exchange function that’s causing the problem, is to overload exchange instead: T exchange(std::nullptr_t) { T oldValue = unpoisoned(); m_poisonedBits = 0; return oldValue; } Are there lots of other cases or is this the one place this warning is triggering?
Note also that this warning is already disabled in WebCore. Don's change extends it to be disabled throughout all WebKit subprojects.
(In reply to Michael Catanzaro from comment #8) > Note also that this warning is already disabled in WebCore. Sure, my comment applies to that original decision, not this patch.
It’s fine with me to consistently turn it off everywhere for now, but I would like to separately consider if we can make a couple simple changes and then turn it back on. From a “port” point of view, lets turn it off since it’s inconvenient to have a warning that other ports aren’t seeing. From an overall health of the project point of view, lets consider turning it back on especially if one of the EWS bots can have a new enough compiler that people will see this warning before landing.
Darin, I've tested your fix and there are no more Wnull-conversion warnings diff --git a/Source/WTF/wtf/Poisoned.h b/Source/WTF/wtf/Poisoned.h index c7371357642..f10dfeb6c18 100644 --- a/Source/WTF/wtf/Poisoned.h +++ b/Source/WTF/wtf/Poisoned.h @@ -200,6 +200,7 @@ public: private: #if ENABLE(POISON) + constexpr static PoisonedBits poison(std::nullptr_t) { return 0; } template<typename U> ALWAYS_INLINE static PoisonedBits poison(U ptr) { return ptr ? bitwise_cast<PoisonedBits>(ptr) ^ key : 0; } template<typename U = T>
(In reply to Michael Catanzaro from comment #8) > Note also that this warning is already disabled in WebCore. Don's change > extends it to be disabled throughout all WebKit subprojects. I don't think that expanding warning suppressions is the right direction, unless we want to have these null conversions spread over whole code base.
Comment on attachment 330876 [details] Patch Attachment 330876 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6023592 New failing tests: webgl/1.0.2/conformance/uniforms/uniform-default-values.html imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-preflight.any.html imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-framesize.html
Created attachment 330944 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
So Don, we should be good to go with Darin's suggestion, right? We could even reenable the warning in WebCore instead of disabling it. (In reply to Darin Adler from comment #10) > From a “port” point of view, lets turn it off since it’s inconvenient to > have a warning that other ports aren’t seeing. AFAIK there has been zero effort to keep CMake and XCode compiler warnings in sync, but all CMake ports should have consistent warning flags.
(In reply to Michael Catanzaro from comment #15) > So Don, we should be good to go with Darin's suggestion, right? We could > even reenable the warning in WebCore instead of disabling it. > > (In reply to Darin Adler from comment #10) > > From a “port” point of view, lets turn it off since it’s inconvenient to > > have a warning that other ports aren’t seeing. > > AFAIK there has been zero effort to keep CMake and XCode compiler warnings > in sync, but all CMake ports should have consistent warning flags. I think I jumped the gun a bit in terms of making the warning global since I saw it enabled in WebCore and it seemed to follow the style guide. I don't think GTK/WPE saw it because GCC is used and this appears to be a Clang only warning. Not sure why Apple didn't see the warning since they also use Clang. I'm doing a build with the constexpr specialization that Darin suggested and I don't see the warning so that is probably the proper fix in this case. I can make a patch once my build is complete.
Created attachment 330950 [details] Patch Implementing Darin's suggestion
Comment on attachment 330950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330950&action=review > Source/WTF/wtf/Poisoned.h:203 > + constexpr static PoisonedBits poison(std::nullptr_t) { return 0; } You also need this specialization for the #else clause because bitwise_cast of nullptr_t isn't guaranteed to give 0 in C++.
Created attachment 330953 [details] Patch nullptr_t specialization for all
(In reply to Don Olmstead from comment #19) > Created attachment 330953 [details] > Patch > > nullptr_t specialization for all I think you uploaded the same patch? Also, I'm sorry C++ is (sometimes) terrible :-)
Comment on attachment 330953 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330953&action=review > Source/WTF/wtf/Poisoned.h:202 > + constexpr static PoisonedBits poison(std::nullptr_t) { return 0; } Its outside the #if ENABLE(POISON) so its in the common implementation
Comment on attachment 330953 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330953&action=review r=me >> Source/WTF/wtf/Poisoned.h:202 >> + constexpr static PoisonedBits poison(std::nullptr_t) { return 0; } > > Its outside the #if ENABLE(POISON) so its in the common implementation Ha! I totally missed this, sorry.
(In reply to JF Bastien from comment #22) > Comment on attachment 330953 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330953&action=review > > r=me > > >> Source/WTF/wtf/Poisoned.h:202 > >> + constexpr static PoisonedBits poison(std::nullptr_t) { return 0; } > > > > Its outside the #if ENABLE(POISON) so its in the common implementation > > Ha! I totally missed this, sorry. Thanks for the review! Will commit after EWS comes back. Also thanks for getting Bell Biv Davoe stuck in my head since I encountered this warning :)
Comment on attachment 330953 [details] Patch Attachment 330953 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6026997 New failing tests: webgl/1.0.2/conformance/uniforms/uniform-default-values.html
Created attachment 330995 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 330953 [details] Patch Clearing flags on attachment: 330953 Committed r226747: <https://trac.webkit.org/changeset/226747>
All reviewed patches have been landed. Closing bug.
<rdar://problem/36423271>