Bug 50985

Summary: [chromium] useless warnings when building on Windows
Product: WebKit Reporter: Evan Martin <evan>
Component: New BugsAssignee: Evan Martin <evan>
Severity: Normal CC: commit-queue, dglazkov, fishd, pkasting
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Description Flags
Patch none

Description Evan Martin 2010-12-13 15:12:16 PST
[chromium] useless warnings when building on Windows
Comment 1 Evan Martin 2010-12-13 15:13:13 PST
Created attachment 76451 [details]
Comment 2 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<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 &)'
            Value=std::pair<WTF::RefPtr<WebCore::FilterEffect>,WTF::HashSet<WebCore::FilterEffect *>>,
            Extractor=WTF::PairFirstExtractor<std::pair<WTF::RefPtr<WebCore::FilterEffect>,WTF::HashSet<WebCore::FilterEffect *>>>,
            RawKeyType=WebCore::FilterEffect *,
            ValueType=std::pair<WTF::RefPtr<WebCore::FilterEffect>,WTF::HashSet<WebCore::FilterEffect *>>,
            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

Here is a trybot run with my patch.  It appears the files weren't rebuilt.  I'm not sure the patch works.  :\

Comment 3 Peter Kasting 2010-12-13 17:29:27 PST
Comment on attachment 76451 [details]

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?
Comment 4 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<Bar, Baz>(blah);
and the warning is saying that if we called
the template it would've chosen would've been different.  I assume this is intentional.
Comment 5 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.
Comment 6 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<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.
Comment 7 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<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);
Comment 8 Adam Barth 2010-12-15 15:15:17 PST
Comment on attachment 76451 [details]

Oops.  Wrong bug.  Sorry for the noise.
Comment 9 Evan Martin 2010-12-16 15:25:47 PST
CC list: need a review, not sure how to get one, can you help?
Comment 10 Darin Fisher (:fishd, Google) 2010-12-16 16:01:02 PST
Comment on attachment 76451 [details]

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
Comment 11 WebKit Commit Bot 2010-12-16 17:52:58 PST
Comment on attachment 76451 [details]

Clearing flags on attachment: 76451

Committed r74221: <http://trac.webkit.org/changeset/74221>
Comment 12 WebKit Commit Bot 2010-12-16 17:53:04 PST
All reviewed patches have been landed.  Closing bug.