50985 2010-12-13 15:12:16 -0800 [chromium] useless warnings when building on Windows 2010-12-16 17:53:04 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 evan evan commit-queue dglazkov fishd pkasting oldest_to_newest 321474 0 evan 2010-12-13 15:12:16 -0800 [chromium] useless warnings when building on Windows 321475 1 76451 evan 2010-12-13 15:13:13 -0800 Created attachment 76451 Patch 321477 2 evan 2010-12-13 15:15:15 -0800 CC'ing Peter since I think he cares about this. Here are the two warnings I'm disabling. One of other warnings I was seeing was a real bug (!) and I've filed that separately (I'm not disabling that warning, obviously). e:\b\build\slave\win\build\src\third_party\webkit\javascriptcore\wtf\text\StringImpl.h(92) : warning C4355: 'this' : used in base member initializer list e:\b\build\slave\win\build\src\third_party\webkit\javascriptcore\wtf\RefPtrHashMap.h(176) : warning C4344: behavior change: use of explicit template arguments results in call to 'WTF::HashTableIterator<Key,Value,Extractor,HashFunctions,Traits,KeyTraits> WTF::HashTable<Key,Value,Extractor,HashFunctions,Traits,KeyTraits>::find<WebCore::FilterEffect*,WTF::RefPtrHashMapRawKeyTranslator<RawKeyType,ValueType,ValueTraits,HashFunctions>>(const T &)' with [ Key=WTF::RefPtr<WebCore::FilterEffect>, Value=std::pair<WTF::RefPtr<WebCore::FilterEffect>,WTF::HashSet<WebCore::FilterEffect *>>, Extractor=WTF::PairFirstExtractor<std::pair<WTF::RefPtr<WebCore::FilterEffect>,WTF::HashSet<WebCore::FilterEffect *>>>, HashFunctions=WTF::PtrHash<WTF::RefPtr<WebCore::FilterEffect>>, Traits=WTF::PairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::FilterEffect>>,WTF::HashTraits<WebCore::SVGFilterBuilder::FilterEffectSet>>, KeyTraits=WTF::HashTraits<WTF::RefPtr<WebCore::FilterEffect>>, RawKeyType=WebCore::FilterEffect *, ValueType=std::pair<WTF::RefPtr<WebCore::FilterEffect>,WTF::HashSet<WebCore::FilterEffect *>>, ValueTraits=WTF::PairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::FilterEffect>>,WTF::HashTraits<WebCore::SVGFilterBuilder::FilterEffectSet>>, T=WebCore::FilterEffect * ] but the regular function 'WTF::HashTableIterator<Key,Value,Extractor,HashFunctions,Traits,KeyTraits> WTF::HashTable<Key,Value,Extractor,HashFunctions,Traits,KeyTraits>::find(const WTF::RefPtr<T> &)' is a better match with Here is a trybot run with my patch. It appears the files weren't rebuilt. I'm not sure the patch works. :\ http://build.chromium.org/p/tryserver.chromium/builders/win/builds/6555/steps/compile/logs/stdio 321570 3 76451 pkasting 2010-12-13 17:29:27 -0800 Comment on attachment 76451 Patch The first warning has been discussed quite a bit in Chromium land and we've repeatedly concluded we shouldn't disable it, but rather annotate safe uses, because it does catch real bugs. I think the same should apply here. I'm not sure what the second warning means. I'd like to understand better. Help? 322079 4 evan 2010-12-14 11:41:32 -0800 We already disable the first warning in most of this code (see 4355 below): JavaScriptCore/JavaScriptCore.gyp/JavaScriptCore.gyp:134: 'msvs_disabled_warnings': [4127, 4355, 4510, 4512, 4610, 4706], All I'm doing here is extending the disabling to other users within WebKit of this header. I believe the second warning is when we're doing something like foo<Bar, Baz>(blah); and the warning is saying that if we called foo(blah); the template it would've chosen would've been different. I assume this is intentional. 322082 5 evan 2010-12-14 11:48:06 -0800 To elaborate on the second point, I believe it is saying that the default template would've used a RefPtr (see the second to last line with "a better match") while the code explicitly uses the raw ptr, presumably to reduce refcount traffic. I think the warning is a pretty weird one. 322141 6 pkasting 2010-12-14 13:03:50 -0800 I won't r- you again, but I still feel like if we disable the "this" warning in JSC and not elsewhere, the right fix is to un-disable it in JSC and fix the instances of it. This one is pretty easy to fix and I'd hope the number of instances are small enough that we can audit them to ensure they're safe. As for the other warning, I concur with your reading of the warning text that it's saying it would normally have passed by const RefPtr<T>& and we're instead passing by const T&. Someone like Darin Adler should probably look to make sure that's the right thing for that code to be doing. 322164 7 evan 2010-12-14 13:33:43 -0800 I attempted to find the origin of that warning-hiding, but it dates back to us landing the gyp files in WebKit. So I think we've effectively always built with that warning disabled. I'd be ok with someone wanting to re-enable that warning and fix webkit, but I think it's out of the scope of this bug. Regarding the RefPtr warning, it's kind of clearer if you look at the code: template<typename T, typename U, typename V, typename W, typename X> inline typename HashMap<RefPtr<T>, U, V, W, X>::iterator HashMap<RefPtr<T>, U, V, W, X>::find(RawKeyType key) { return m_impl.template find<RawKeyType, RawKeyTranslator>(key); } 322841 8 76451 abarth 2010-12-15 15:15:17 -0800 Comment on attachment 76451 Patch Oops. Wrong bug. Sorry for the noise. 323456 9 evan 2010-12-16 15:25:47 -0800 CC list: need a review, not sure how to get one, can you help? 323488 10 76451 fishd 2010-12-16 16:01:02 -0800 Comment on attachment 76451 Patch WebKit code uses 'this' in initializer lists pretty freely. I think it is reasonable to suppress this warning consistently in WebKit. I also think it is reasonable to raise this issue for discussion on #webkit to see if folks have interest in introducing a macro like Chromium's ALLOW_THIS_IN_INITIALIZER_LIST. 323546 11 76451 commit-queue 2010-12-16 17:52:58 -0800 Comment on attachment 76451 Patch Clearing flags on attachment: 76451 Committed r74221: <http://trac.webkit.org/changeset/74221> 323547 12 commit-queue 2010-12-16 17:53:04 -0800 All reviewed patches have been landed. Closing bug. 76451 2010-12-13 15:13:13 -0800 2010-12-16 17:52:58 -0800 Patch bug-50985-20101213151219.patch text/plain 1579 evan ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCA0Nzg1ZWFlNzcwZmQ5MDE4NGM5NTIxYjRhMWRhN2UxN2Y1NDAxYzcwLi5j OWZiMzQ3Y2U3NGRhNWVmZmUyNjVhYzRkM2U1NzYzYWU5ZWM2MWMxIDEwMDY0NAotLS0gYS9KYXZh U2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0x LDMgKzEsMTQgQEAKKzIwMTAtMTItMTMgIEV2YW4gTWFydGluICA8ZXZhbkBjaHJvbWl1bS5vcmc+ CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW2Nocm9t aXVtXSB1c2VsZXNzIHdhcm5pbmdzIHdoZW4gYnVpbGRpbmcgb24gV2luZG93cworICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTA5ODUKKworICAgICAgICBE aXNhYmxlIHNvbWUgY29tcGlsZXIgd2FybmluZ3MgdGhhdCBhcmVuJ3QgaW5kaWNhdGl2ZSBvZiBy ZWFsIHByb2JsZW1zLgorCisgICAgICAgICogSmF2YVNjcmlwdENvcmUuZ3lwL0phdmFTY3JpcHRD b3JlLmd5cDoKKwogMjAxMC0xMi0xMyAgU3RldmUgRmFsa2VuYnVyZyAgPHNmYWxrZW5AYXBwbGUu Y29tPgogCiAgICAgICAgIFdpbmRvd3MgcHJvZHVjdGlvbiBidWlsZCBmaXguCmRpZmYgLS1naXQg YS9KYXZhU2NyaXB0Q29yZS9KYXZhU2NyaXB0Q29yZS5neXAvSmF2YVNjcmlwdENvcmUuZ3lwIGIv SmF2YVNjcmlwdENvcmUvSmF2YVNjcmlwdENvcmUuZ3lwL0phdmFTY3JpcHRDb3JlLmd5cAppbmRl eCAzMjI3MGU4MWFjZDY0MjRhNjQzNTc5ZjhkYmIzZmUzZGI2MDM4Nzk4Li5kNzVmY2NmYzIzODNk MzJmZTFhNTA2ODI3NWVjYTRkNDU2Y2U2ZWRmIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29yZS9K YXZhU2NyaXB0Q29yZS5neXAvSmF2YVNjcmlwdENvcmUuZ3lwCisrKyBiL0phdmFTY3JpcHRDb3Jl L0phdmFTY3JpcHRDb3JlLmd5cC9KYXZhU2NyaXB0Q29yZS5neXAKQEAgLTEyNSw2ICsxMjUsMTYg QEAKICAgICAgICAgICAnLi4vJywKICAgICAgICAgICAnLi4vd3RmJywKICAgICAgICAgXSwKKyAg ICAgICAgIyBTb21lIHdhcm5pbmdzIG9jY3VyIGluIEpTQyBoZWFkZXJzLCBzbyB0aGV5IG11c3Qg YWxzbyBiZSBkaXNhYmxlZAorICAgICAgICAjIGluIHRhcmdldHMgdGhhdCB1c2UgSlNDLgorICAg ICAgICAnbXN2c19kaXNhYmxlZF93YXJuaW5ncyc6IFsKKyAgICAgICAgICAjIERvbid0IGNvbXBs YWluIGFib3V0IGNhbGxpbmcgc3BlY2lmaWMgdmVyc2lvbnMgb2YgdGVtcGxhdGl6ZWQKKyAgICAg ICAgICAjIGZ1bmN0aW9ucyAoZS5nLiBpbiBSZWZQdHJIYXNoTWFwLmgpLgorICAgICAgICAgIDQz NDQsCisgICAgICAgICAgIyBEb24ndCBjb21wbGFpbiBhYm91dCB1c2luZyAidGhpcyIgaW4gYW4g aW5pdGlhbGl6ZXIgbGlzdAorICAgICAgICAgICMgKGUuZy4gaW4gU3RyaW5nSW1wbC5oKS4KKyAg ICAgICAgICA0MzU1LAorICAgICAgICBdLAogICAgICAgfSwKICAgICAgICdleHBvcnRfZGVwZW5k ZW50X3NldHRpbmdzJzogWwogICAgICAgICAnd3RmX2NvbmZpZycsCg==