Bug 209146 – Fix many warnings with Clang 7.0 on GTK x86-64 in Debug.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Charlie Turner]

Fix many warnings with Clang 7.0 on GTK x86-64 in Debug.

[Charlie Turner]

Created attachment 393673 [details]
Patch

Fix a load of warnings I was seeing on GTK with Clang 7.0. Hopefully the EWS will alert me to any issues with RELEASE_LOG_DISABLED=0, which is Cocoa specific at the moment.

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Charlie Turner]

Created attachment 393683 [details]
Patch

Now it becomes visible why there were several seemingly unused this captures in lambdas, the RELEASE_LOG macros implicity depend on this for the class-local logger() method to resovllve. When https://bugs.webkit.org/show_bug.cgi?id=195182  lands these logging macros will become less of a maintenance burden for non-Cocoa ports. .

[Charlie Turner]

Created attachment 393690 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 393690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393690&action=review

> Source/WTF/wtf/LoggerHelper.h:89
> +#define ALWAYS_LOG(id, ...)  (UNUSED_PARAM(id))
> +#define ERROR_LOG(id, ...)   (UNUSED_PARAM(id))
> +#define WARNING_LOG(id, ...) (UNUSED_PARAM(id))
> +#define NOTICE_LOG(id, ...)  (UNUSED_PARAM(id))
> +#define INFO_LOG(id, ...)    (UNUSED_PARAM(id))
> +#define DEBUG_LOG(id, ...)   (UNUSED_PARAM(id))
> +#define WILL_LOG(_level_)    false

'id' is a reserved word in Obj-C. I would suggest something else (it's the channel name).

[Charlie Turner]

Created attachment 393742 [details]
Patch

Avoid using the uid=1000(cht) gid=1001(cht) groups=1001(cht),24(cdrom),25(floppy),27(sudo),29(audio),30(dip),44(video),46(plugdev),109(netdev),113(bluetooth),117(scanner),131(wireshark) identifier which is reserved in Obj-C, explain non-obvious things in ChangeLogs now that the dust has settled in the EWS.

[Charlie Turner]

Shell expansion is fun. The gobbledygook was intially just ".... `id` ...", and bash kindly told the world of my group subscriptions :-)

[Charlie Turner]

+fast/hidpi/image-srcset-relative-svg-canvas-2x.html		reference images diff (0.01%)	image	pass	history


I'm not convinced the ios-wk2 failure is caused by my patch

[Darin Adler]

Comment on attachment 393742 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393742&action=review

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.h:38
> -    const char* name() final { return "Video"; }
> +    const char* name() { return "Video"; }

This seems strange. Why would we need this function if it’s not overriding a virtual function?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:303
> +        auto encoder = createEncoder();
> +        if (encoder) {

I suggest putting the definition inside the if:

    if (auto encoder = createEncoder()) {

> Source/WebCore/testing/Internals.cpp:5517
>  String Internals::encodedPreferenceValue(const String& domain, const String& key)
>  {
> +    UNUSED_PARAM(domain);
> +    UNUSED_PARAM(key);
>      return emptyString();
>  }
>  
>  String Internals::getUTIFromMIMEType(const String& mimeType)
>  {
> +    UNUSED_PARAM(mimeType);
>      return emptyString();
>  }
>  
>  String Internals::getUTIFromTag(const String& tagClass, const String& tag, const String& conformingToUTI)
>  {
> +    UNUSED_PARAM(tagClass);
> +    UNUSED_PARAM(tag);
> +    UNUSED_PARAM(conformingToUTI);
>      return emptyString();
>  }

I suggest taking out the argument names instead of using UNUSED_PARAM, but either is OK I suppose.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:67
> +#if PLATFORM(COCOA)
>      , m_isHTTPSUpgradeEnabled(isHTTPSUpgradeEnabled)
> +#endif

Don’t we need UNUSED_PARAM for non-PLATFORM(COCOA)?

[Charlie Turner]

(In reply to Darin Adler from comment #9)
> Comment on attachment 393742 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393742&action=review
> 
> > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.h:38
> > -    const char* name() final { return "Video"; }
> > +    const char* name() { return "Video"; }
> 
> This seems strange. Why would we need this function if it’s not overriding a
> virtual function?

It is overriding a virtual function. I incorrectly assumed a class-final specifier implied that the overridden virtuals from the base were implied final in the derived class. Fixed by putting the final back.

> 
> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:303
> > +        auto encoder = createEncoder();
> > +        if (encoder) {
> 
> I suggest putting the definition inside the if:
> 
>     if (auto encoder = createEncoder()) {

Done.

> 
> > Source/WebCore/testing/Internals.cpp:5517
> >  String Internals::encodedPreferenceValue(const String& domain, const String& key)
> >  {
> > +    UNUSED_PARAM(domain);
> > +    UNUSED_PARAM(key);
> >      return emptyString();
> >  }
> >  
> >  String Internals::getUTIFromMIMEType(const String& mimeType)
> >  {
> > +    UNUSED_PARAM(mimeType);
> >      return emptyString();
> >  }
> >  
> >  String Internals::getUTIFromTag(const String& tagClass, const String& tag, const String& conformingToUTI)
> >  {
> > +    UNUSED_PARAM(tagClass);
> > +    UNUSED_PARAM(tag);
> > +    UNUSED_PARAM(conformingToUTI);
> >      return emptyString();
> >  }
> 
> I suggest taking out the argument names instead of using UNUSED_PARAM, but
> either is OK I suppose.

I will follow your suggestion, I figured leaving the names in would save a little time if they ever were to be fleshed out, but it's an eye-sore.

> 
> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:67
> > +#if PLATFORM(COCOA)
> >      , m_isHTTPSUpgradeEnabled(isHTTPSUpgradeEnabled)
> > +#endif
> 
> Don’t we need UNUSED_PARAM for non-PLATFORM(COCOA)?

Yes, I am not warning free yet, and this one slipped my eye.

Remaining sources of warning litter are,

  1. -Wcast-align, a check that appears not to peel back all the layers of allocations in WebKit and see the original alignment restrictions made, tempting to disable it.

  2. Warnings about unused `this` captures in lambdas, they're only used in !RELEASE_LOG_DISABLED builds, when the macro expands inside the lambda... It's ongoing work to make this logging macro port-agnostic, a systemd backend recently landed, but my embedded device does not use systemd, so I will need to think about an alternative in the meantime.

[Konstantin Tokarev]

Comment on attachment 393742 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393742&action=review

>>> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.h:38
>>> +    const char* name() { return "Video"; }
>> 
>> This seems strange. Why would we need this function if it’s not overriding a virtual function?
> 
> It is overriding a virtual function. I incorrectly assumed a class-final specifier implied that the overridden virtuals from the base were implied final in the derived class. Fixed by putting the final back.

Final specifier on class indeed does that, however missing final on method hurts readability

[Charlie Turner]

Created attachment 393851 [details]
Patch

[Darin Adler]

Comment on attachment 393742 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393742&action=review

>>>> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCapturer.h:38
>>>> +    const char* name() { return "Video"; }
>>> 
>>> This seems strange. Why would we need this function if it’s not overriding a virtual function?
>> 
>> It is overriding a virtual function. I incorrectly assumed a class-final specifier implied that the overridden virtuals from the base were implied final in the derived class. Fixed by putting the final back.
> 
> Final specifier on class indeed does that, however missing final on method hurts readability

The reason to include *either* final or override to to state explicitly that we are overriding a function and make it an error if there is no virtual function in a base class.

We choose to include only "final" in a case like this. We could have chosen as a matter of coding style to include "override" or both "override" and "final". All of those would have the same effect since the class is already marked final.

But listing nothing at all means we miss out on a warning or error if the function in the base class is removed or renamed.

[Darin Adler]

Comment on attachment 393851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393851&action=review

> Source/WebCore/loader/cache/CachedResource.cpp:336
> +#if !RELEASE_LOG_DISABLED
>              auto& frame = frameRef.get();
>              RELEASE_LOG_IF(loggingAllowed, Network, "%p - [pageID=%" PRIu64 ", frameID=%" PRIu64 "] CachedResource::load: Unable to create SubresourceLoader", this, PAGE_ID(frame), FRAME_ID(frame));
> +#endif

I personally would have just repeated frameRef.get() three times, making the RELEASE_LOG_IF line a bit longer to keep this a little more tidy.

[Charlie Turner]

(In reply to Darin Adler from comment #14)
> Comment on attachment 393851 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393851&action=review
> 
> > Source/WebCore/loader/cache/CachedResource.cpp:336
> > +#if !RELEASE_LOG_DISABLED
> >              auto& frame = frameRef.get();
> >              RELEASE_LOG_IF(loggingAllowed, Network, "%p - [pageID=%" PRIu64 ", frameID=%" PRIu64 "] CachedResource::load: Unable to create SubresourceLoader", this, PAGE_ID(frame), FRAME_ID(frame));
> > +#endif
> 
> I personally would have just repeated frameRef.get() three times, making the
> RELEASE_LOG_IF line a bit longer to keep this a little more tidy.

Done.

Thanks for the review, patch for landing coming up.

[Charlie Turner]

Created attachment 393964 [details]
Patch

[Charlie Turner]

Created attachment 393966 [details]
Patch

Accidentally cleared the r+ status uploaded the last patch. Manually added the Reviewed by lines for the commit queue

[Charlie Turner]

Created attachment 393970 [details]
Patch

r258635 landed unreviewed while this was being reviewed, and conflicted with its approach of eliminating unused parameter warnings. They were using UNUSED_PARAM(), I am deleting the parameter names as suggested by Darin. So, in affect this patch is now acting as a revert of r258635 as well as a more general warnings suppression patch.

[EWS]

Committed r258698: <https://trac.webkit.org/changeset/258698>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 393970 [details].