Bug 50985 - [chromium] useless warnings when building on Windows
[chromium] useless warnings when building on Windows
 Status: RESOLVED FIXED None WebKit Unclassified New Bugs (show other bugs) 528+ (Nightly build) Other OS X 10.5 P2 Normal Evan Martin

 Reported: 2010-12-13 15:12 PST by Evan Martin 2010-12-16 17:53 PST (History) 4 users (show) commit-queue dglazkov fishd pkasting

Attachments
Patch (1.54 KB, patch)
2010-12-13 15:13 PST, Evan Martin
no flags Details | Formatted Diff | Diff

 Note You need to log in before you can comment on or make changes to this bug.
 Evan Martin 2010-12-13 15:12:16 PST [chromium] useless warnings when building on Windows Evan Martin 2010-12-13 15:13:13 PST Created attachment 76451 [details] Patch Evan Martin 2010-12-13 15:15:15 PST 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 WTF::HashTable::find>(const T &)' with [ Key=WTF::RefPtr, Value=std::pair,WTF::HashSet>, Extractor=WTF::PairFirstExtractor,WTF::HashSet>>, HashFunctions=WTF::PtrHash>, Traits=WTF::PairHashTraits>,WTF::HashTraits>, KeyTraits=WTF::HashTraits>, RawKeyType=WebCore::FilterEffect *, ValueType=std::pair,WTF::HashSet>, ValueTraits=WTF::PairHashTraits>,WTF::HashTraits>, T=WebCore::FilterEffect * ] but the regular function 'WTF::HashTableIterator WTF::HashTable::find(const WTF::RefPtr &)' 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 Peter Kasting 2010-12-13 17:29:27 PST Comment on attachment 76451 [details] 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? Evan Martin 2010-12-14 11:41:32 PST 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(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. Evan Martin 2010-12-14 11:48:06 PST 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. Peter Kasting 2010-12-14 13:03:50 PST 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& 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. Evan Martin 2010-12-14 13:33:43 PST 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 inline typename HashMap, U, V, W, X>::iterator HashMap, U, V, W, X>::find(RawKeyType key) { return m_impl.template find(key); } Adam Barth 2010-12-15 15:15:17 PST Comment on attachment 76451 [details] Patch Oops. Wrong bug. Sorry for the noise. Evan Martin 2010-12-16 15:25:47 PST CC list: need a review, not sure how to get one, can you help? Darin Fisher (:fishd, Google) 2010-12-16 16:01:02 PST Comment on attachment 76451 [details] 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. WebKit Commit Bot 2010-12-16 17:52:58 PST Comment on attachment 76451 [details] Patch Clearing flags on attachment: 76451 Committed r74221:  WebKit Commit Bot 2010-12-16 17:53:04 PST All reviewed patches have been landed. Closing bug.