Bug 50985 – [chromium] useless warnings when building on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Evan Martin]

[chromium] useless warnings when building on Windows

[Evan Martin]

Created attachment 76451 [details]
Patch

[Evan Martin]

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

[Peter Kasting]

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]

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.

[Evan Martin]

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]

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.

[Evan Martin]

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);
    }

[Adam Barth]

Comment on attachment 76451 [details]
Patch

Oops.  Wrong bug.  Sorry for the noise.

[Evan Martin]

CC list: need a review, not sure how to get one, can you help?

[Darin Fisher (:fishd, Google)]

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]

Comment on attachment 76451 [details]
Patch

Clearing flags on attachment: 76451

Committed r74221: <http://trac.webkit.org/changeset/74221>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.