Some of these are new warnings detected by GCC 10. Others are just newly-introduced. Turns out that GCC 10 is a very easy upgrade for us. There is usually a huge spam of new warnings when upgrading to a new major version of GCC. Not this time.
Created attachment 394303 [details] Patch
Comment on attachment 394303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394303&action=review > Source/WebCore/dom/Element.cpp:3803 > - if (auto* animationData = animationRareData()) > + if (animationRareData()) > return &animationRareData()->webAnimations(); This is the wrong fix to this error. The right fix is to use the local variable on the next line, so the compiler doesn’t have to figure out the function doesn’t have to be called twice. Same for the other 5 instances below. > Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp:114 > + bool monitoringWheelEvents = isMonitoringWheelEvents(); > if (monitoringWheelEvents) No need for the local variable at all. > Source/WebCore/platform/network/HTTPParsers.h:175 > - return WTFMove(set); > + return set; This change seems wrong. The return value optimization doesn’t apply when the local variable type is HashSet and then return value type is Optional<HashSet>. Is GCC really giving a warning here? > Source/WebKit/UIProcess/API/C/WKPage.cpp:1208 > +ALLOW_DEPRECATED_DECLARATIONS_BEGIN > auto loaderClient = makeUnique<LoaderClient>(wkClient); > +ALLOW_DEPRECATED_DECLARATIONS_END What’s deprecated? This doesn’t make sense to me. > Source/WebKit/UIProcess/API/C/WKPage.cpp:1292 > +ALLOW_DEPRECATED_DECLARATIONS_BEGIN > toImpl(pageRef)->setPolicyClient(makeUnique<PolicyClient>(wkClient)); > +ALLOW_DEPRECATED_DECLARATIONS_END Ditto.
(In reply to Darin Adler from comment #2) > This is the wrong fix to this error. The right fix is to use the local > variable on the next line, so the compiler doesn’t have to figure out the > function doesn’t have to be called twice. Same for the other 5 instances > below. Oops, yes, good catch! > > Source/WebCore/page/scrolling/ThreadedScrollingTree.cpp:114 > > + bool monitoringWheelEvents = isMonitoringWheelEvents(); > > if (monitoringWheelEvents) > > No need for the local variable at all. Oops, yes, good catch! > > Source/WebCore/platform/network/HTTPParsers.h:175 > > - return WTFMove(set); > > + return set; > > This change seems wrong. The return value optimization doesn’t apply when > the local variable type is HashSet and then return value type is > Optional<HashSet>. Is GCC really giving a warning here? Are you sure that return value optimization doesn't apply? Yes, GCC warns that the move is redundant. I would not bet against GCC here. :) afaik it's never appropriate to use WTFMove() in a return statement? > > Source/WebKit/UIProcess/API/C/WKPage.cpp:1208 > > +ALLOW_DEPRECATED_DECLARATIONS_BEGIN > > auto loaderClient = makeUnique<LoaderClient>(wkClient); > > +ALLOW_DEPRECATED_DECLARATIONS_END > > What’s deprecated? This doesn’t make sense to me. GCC 10 is getting really aggressive with -Wdeprecated-declarations: [74/216] Building CXX object Source/WebKit/CMakeFiles/Web...es/WebKit/unified-sources/UnifiedSource-88d1702b-17.cpp.o In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-88d1702b-17.cpp:1: /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/API/C/WKPage.cpp: In function ‘void WKPageSetPageLoaderClient(WKPageRef, const WKPageLoaderClientBase*)’: /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/API/C/WKPage.cpp:1206:36: warning: ‘void WKPageSetPageLoaderClient(WKPageRef, const WKPageLoaderClientBase*)’ is deprecated: use WKPageSetPageNavigationClient [-Wdeprecated-declarations] 1206 | auto loaderClient = makeUnique<LoaderClient>(wkClient); | ^~~~~~~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/API/C/WKPage.cpp:1058:6: note: declared here 1058 | void WKPageSetPageLoaderClient(WKPageRef pageRef, const WKPageLoaderClientBase* wkClient) | ^~~~~~~~~~~~~~~~~~~~~~~~~ I agree it doesn't make a ton of sense, but it's easy to avoid. > > Source/WebKit/UIProcess/API/C/WKPage.cpp:1292 > > +ALLOW_DEPRECATED_DECLARATIONS_BEGIN > > toImpl(pageRef)->setPolicyClient(makeUnique<PolicyClient>(wkClient)); > > +ALLOW_DEPRECATED_DECLARATIONS_END > > Ditto. /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/API/C/WKPage.cpp: In function ‘void WKPageSetPagePolicyClient(WKPageRef, const WKPagePolicyClientBase*)’: /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/API/C/WKPage.cpp:1288:49: warning: ‘void WKPageSetPagePolicyClient(WKPageRef, const WKPagePolicyClientBase*)’ is deprecated: use WKPageSetPageNavigationClient [-Wdeprecated-declarations] 1288 | toImpl(pageRef)->setPolicyClient(makeUnique<PolicyClient>(wkClient)); | ^~~~~~~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/API/C/WKPage.cpp:1223:6: note: declared here 1223 | void WKPageSetPagePolicyClient(WKPageRef pageRef, const WKPagePolicyClientBase* wkClient) | ^~~~~~~~~~~~~~~~~~~~~~~~~
(In reply to Michael Catanzaro from comment #3) > Are you sure that return value optimization doesn't apply? Yes, GCC warns > that the move is redundant. I would not bet against GCC here. :) afaik it's > never appropriate to use WTFMove() in a return statement? It looks like this: [1078/1697] Building CXX object Source/WebCore/CMakeFiles...es/WebCore/unified-sources/UnifiedSource-c57e08af-1.cpp.o In file included from DerivedSources/ForwardingHeaders/wtf/FastMalloc.h:26, from /home/mcatanzaro/Projects/WebKit/Source/WebCore/config.h:56, from /home/mcatanzaro/Projects/WebKit/Source/WebCore/loader/AdClickAttribution.cpp:26, from DerivedSources/WebCore/unified-sources/UnifiedSource-c57e08af-1.cpp:1: /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/network/HTTPParsers.h: In instantiation of ‘WTF::Optional<WTF::HashSet<WTF::String, HashType> > WebCore::parseAccessControlAllowList(const WTF::String&) [with HashType = WTF::StringHash]’: /home/mcatanzaro/Projects/WebKit/Source/WebCore/loader/CrossOriginPreflightResultCache.cpp:57:115: required from here DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:495:64: warning: redundant move in return statement [-Wredundant-move] 495 | #define WTFMove(value) std::move<WTF::CheckMoveParameter>(value) | ^ /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/network/HTTPParsers.h:175:12: note: in expansion of macro ‘WTFMove’ 175 | return WTFMove(set); | ^~~~~~~ DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:495:64: note: remove ‘std::move’ call 495 | #define WTFMove(value) std::move<WTF::CheckMoveParameter>(value) | ^ /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/network/HTTPParsers.h:175:12: note: in expansion of macro ‘WTFMove’ 175 | return WTFMove(set); | ^~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/network/HTTPParsers.h: In instantiation of ‘WTF::Optional<WTF::HashSet<WTF::String, HashType> > WebCore::parseAccessControlAllowList(const WTF::String&) [with HashType = WTF::ASCIICaseInsensitiveHash]’: /home/mcatanzaro/Projects/WebKit/Source/WebCore/loader/CrossOriginPreflightResultCache.cpp:61:141: required from here DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:495:64: warning: redundant move in return statement [-Wredundant-move] 495 | #define WTFMove(value) std::move<WTF::CheckMoveParameter>(value) | ^ /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/network/HTTPParsers.h:175:12: note: in expansion of macro ‘WTFMove’ 175 | return WTFMove(set); | ^~~~~~~ DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:495:64: note: remove ‘std::move’ call 495 | #define WTFMove(value) std::move<WTF::CheckMoveParameter>(value) | ^ /home/mcatanzaro/Projects/WebKit/Source/WebCore/platform/network/HTTPParsers.h:175:12: note: in expansion of macro ‘WTFMove’ 175 | return WTFMove(set); | ^~~~~~~ But it prints in many more places than that, again for each unified source bundle that includes HTTPParsers.h. -Wredundant-move is one of the most common warnings I see, because clang doesn't have this warning (clang only has -Wpessimizing-move) so Apple devs don't notice it. I have a lot of previous commits removing this warning all over the place.
Created attachment 394308 [details] Patch
(In reply to Michael Catanzaro from comment #3) > (In reply to Darin Adler from comment #2) > > > Source/WebCore/platform/network/HTTPParsers.h:175 > > > - return WTFMove(set); > > > + return set; > > > > This change seems wrong. The return value optimization doesn’t apply when > > the local variable type is HashSet and then return value type is > > Optional<HashSet>. Is GCC really giving a warning here? > > Are you sure that return value optimization doesn't apply? Yes, GCC warns > that the move is redundant. I would not bet against GCC here. :) OK. > afaik it's > never appropriate to use WTFMove() in a return statement? I’m sure that broad generalization is wrong. You could have a return statement calling a function that takes an rvalue reference.
(In reply to Michael Catanzaro from comment #3) > > > Source/WebKit/UIProcess/API/C/WKPage.cpp:1208 > > > +ALLOW_DEPRECATED_DECLARATIONS_BEGIN > > > auto loaderClient = makeUnique<LoaderClient>(wkClient); > > > +ALLOW_DEPRECATED_DECLARATIONS_END > > > > What’s deprecated? This doesn’t make sense to me. > > GCC 10 is getting really aggressive with -Wdeprecated-declarations: > > [74/216] Building CXX object > Source/WebKit/CMakeFiles/Web...es/WebKit/unified-sources/UnifiedSource- > 88d1702b-17.cpp.o > In file included from > DerivedSources/WebKit/unified-sources/UnifiedSource-88d1702b-17.cpp:1: > /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/API/C/WKPage.cpp: > In function ‘void WKPageSetPageLoaderClient(WKPageRef, const > WKPageLoaderClientBase*)’: > /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/API/C/WKPage.cpp: > 1206:36: warning: ‘void WKPageSetPageLoaderClient(WKPageRef, const > WKPageLoaderClientBase*)’ is deprecated: use WKPageSetPageNavigationClient > [-Wdeprecated-declarations] > 1206 | auto loaderClient = makeUnique<LoaderClient>(wkClient); > | ^~~~~~~~~~~~ > /home/mcatanzaro/Projects/WebKit/Source/WebKit/UIProcess/API/C/WKPage.cpp: > 1058:6: note: declared here > 1058 | void WKPageSetPageLoaderClient(WKPageRef pageRef, const > WKPageLoaderClientBase* wkClient) > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > I agree it doesn't make a ton of sense, but it's easy to avoid. Where’s the call to WKPageSetPageLoaderClient? Seems like this beyond "aggressive", and just a bug, but maybe I’m missing something.
Comment on attachment 394308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394308&action=review Thanks, you resolved most of the issues I brought up in the first patch. > Source/WTF/wtf/ConcurrentBuffer.h:70 > - memcpy(newArray->data, array->data, sizeof(T) * array->size); > + memcpy(static_cast<void*>(newArray->data), array->data, sizeof(T) * array->size); This is peculiar enough that I think we need a comment in the code, not just in the change log. Might also be better to use warning-suppression macros instead of the static_cast. That might be more self-explanatory and possibly could allow us to omit the comment. > Source/WebKit/UIProcess/API/C/WKPage.cpp:1208 > +ALLOW_DEPRECATED_DECLARATIONS_BEGIN > auto loaderClient = makeUnique<LoaderClient>(wkClient); > +ALLOW_DEPRECATED_DECLARATIONS_END I’m still not OK with this change. I realize now that this entire function is implementing something deprecated. What I would expect is: ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN ALLOW_DEPRECATED_IMPLEMENTATIONS_END around the entire function. Putting ALLOW_DEPRECATED_DECLARATIONS_BEGIN around this one line makes no sense. > Source/WebKit/UIProcess/API/C/WKPage.cpp:1292 > +ALLOW_DEPRECATED_DECLARATIONS_BEGIN > toImpl(pageRef)->setPolicyClient(makeUnique<PolicyClient>(wkClient)); > +ALLOW_DEPRECATED_DECLARATIONS_END Ditto.
(In reply to Darin Adler from comment #6) > I’m sure that broad generalization is wrong. You could have a return > statement calling a function that takes an rvalue reference. Well yes, of course that's fine. I just mean the final returned result should not use WTFMove(). (In reply to Darin Adler from comment #8) > > Source/WTF/wtf/ConcurrentBuffer.h:70 > > - memcpy(newArray->data, array->data, sizeof(T) * array->size); > > + memcpy(static_cast<void*>(newArray->data), array->data, sizeof(T) * array->size); > > This is peculiar enough that I think we need a comment in the code, not just > in the change log. Might also be better to use warning-suppression macros > instead of the static_cast. That might be more self-explanatory and possibly > could allow us to omit the comment. Hm, your choice! This is what I've historically been doing to suppress -Wclass-memaccess in WebKit, though, and I think it's idiomatic (casting the first arg to memcpy() should be familiar to developers who have seen it used to suppress this warning before): The -Wclass-memaccess option is enabled by -Wall. Explicitly casting the pointer to the class object to "void *" or to a type that can be safely accessed by the raw memory function suppresses the warning. Note that the warning is relatively new (GCC 8, I think) and Clang doesn't have it at all, so I agree the idiom won't be familiar to most developers. And ideally, developers wouldn't write code that triggers this warning in the first place. But we unfortunately have several other suspicious places in WebKit that do. You can find them with: $ cd WebKit/Source/ $ git grep 'static_cast<void\*>' The lines that use memcpy, memmove, memset, etc. are there for this warning. Full docs for this warning: -Wclass-memaccess (C++ and Objective-C++ only) Warn when the destination of a call to a raw memory function such as "memset" or "memcpy" is an object of class type, and when writing into such an object might bypass the class non-trivial or deleted constructor or copy assignment, violate const-correctness or encapsulation, or corrupt virtual table pointers. Modifying the representation of such objects may violate invariants maintained by member functions of the class. For example, the call to "memset" below is undefined because it modifies a non- trivial class object and is, therefore, diagnosed. The safe way to either initialize or clear the storage of objects of such types is by using the appropriate constructor or assignment operator, if one is available. std::string str = "abc"; memset (&str, 0, sizeof str); The -Wclass-memaccess option is enabled by -Wall. Explicitly casting the pointer to the class object to "void *" or to a type that can be safely accessed by the raw memory function suppresses the warning. > > Source/WebKit/UIProcess/API/C/WKPage.cpp:1208 > > +ALLOW_DEPRECATED_DECLARATIONS_BEGIN > > auto loaderClient = makeUnique<LoaderClient>(wkClient); > > +ALLOW_DEPRECATED_DECLARATIONS_END > > I’m still not OK with this change. I realize now that this entire function > is implementing something deprecated. What I would expect is: > > ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN > ALLOW_DEPRECATED_IMPLEMENTATIONS_END > > around the entire function. Putting ALLOW_DEPRECATED_DECLARATIONS_BEGIN > around this one line makes no sense. I agree, the warning does not make sense. :S Using ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN won't do anything, though, because this is not a -Wdeprecated-implementations warning, it's really -Wdeprecated-declarations. (GCC does not have -Wdeprecated-implementations.) I could add magic in Compiler.h to have ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN suppress -Wdeprecated-declarations instead of -Wdeprecated-implementations when the compiler is GCC, if you want.
Darin, any further comments? If not, I'll add a comment about the static_cast<void*> to address your concern regarding its mysteriousness. I'll prefer to use that rather than add a new IGNORE_WARNINGS macro because static_cast<void*> is idiomatic. Then I won't make any changes to ALLOW_DEPRECATED_DECLARATIONS_BEGIN/END since it seems nicer to use the macro corresponding to the warning we actually want to suppress, rather than change ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN/END to suppress -Wdeprecated-declarations. But if you have other preferences, of course I'll implement what you prefer.
(In reply to Michael Catanzaro from comment #10) > If not, I'll add a comment about the static_cast<void*> to address your > concern regarding its mysteriousness. I'll prefer to use that rather than > add a new IGNORE_WARNINGS macro because static_cast<void*> is idiomatic. Not asking you to add a new IGNORE_WARNINGS macro. Could use IGNORE_GCC_WARNINGS_BEGIN/END here. I don’t know what you mean by "is idiomatic", but despite not fully understanding that I won’t try to stand in the way with landing a workaround for the warning as long as there’s a comment. I think, though, that in general, when trying to keep WebKit working with multiple compilers, changing code to sidestep warnings has been slightly more fragile and hard to understand than finding a way to turn the warnings off. > Then I won't make any changes to ALLOW_DEPRECATED_DECLARATIONS_BEGIN/END > since it seems nicer to use the macro corresponding to the warning we > actually want to suppress, rather than change > ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN/END to suppress > -Wdeprecated-declarations. The point of these macros is to create some compiler independence. The fact that on GCC when we try to compile an implementation of a function that we marked as deprecated, we get a "deprecated declaration" warning, is the kind of thing we are trying to abstract away. As I understand it, what this code *does* that is worth warning about is that it *implements* something that is deprecated. So the compiler-independent macro to use is ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN/END. In practice, I don’t know what that will have to expand to for each compiler to make that work, but the point of that macro is to abstract that away. It’s straight-up bizarre to use ALLOW_DEPRECATED_DECLARATIONS_BEGIN/END around a line of code that does *not* use something that’s deprecated. I don’t care if it quiets GCC down or not. It does not make sense. So that’s my thinking on how to quiet these GCC warnings.
FWIW, I'm pretty sure the -Wdeprecated-declarations warning is a GCC bug. It's not attempting to warn us that we are implementing a deprecated API. It's just getting really confused and somehow accidentally doing that. So my suggestion is to keep using ALLOW_DEPRECATED_DECLARATIONS_BEGIN/END and just add a comment that it's addressing a compiler bug and should be removed in the future. I think this is OK since it's only two places in one file in our entire codebase. But of course I can change it to ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN/END if you want. It's just not what GCC is really trying to warn about.
Created attachment 394665 [details] Patch
Comment on attachment 394665 [details] Patch BTW thanks for the thorough reviews, as usual. Helps get the code quality as high as we can. ;)
Committed r259093: <https://trac.webkit.org/changeset/259093> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394665 [details].
<rdar://problem/60943111>