Bug 192743 - Use warning-ignoring macros more consistently and simply
Summary: Use warning-ignoring macros more consistently and simply
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-15 13:30 PST by Darin Adler
Modified: 2018-12-17 15:29 PST (History)
7 users (show)

See Also:


Attachments
Patch (35.47 KB, patch)
2018-12-15 13:45 PST, Darin Adler
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2018-12-15 13:30:20 PST
Use warning-ignoring macros more consistently and simply
Comment 1 Darin Adler 2018-12-15 13:45:13 PST
Created attachment 357405 [details]
Patch
Comment 2 Mark Lam 2018-12-15 16:04:17 PST
Comment on attachment 357405 [details]
Patch

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

r=me

> Source/WebKitLegacy/mac/WebCoreSupport/WebAlternativeTextClient.h:35
> +    explicit WebAlternativeTextClient(WebView *);

Remove space between "WebView" and "*".

> Source/WebKitLegacy/mac/WebCoreSupport/WebAlternativeTextClient.h:54
> +    WebView *m_webView;

Shouldn't this be "WebView* m_webView;"?  Why move the placement of the space?
Comment 3 Darin Adler 2018-12-15 16:05:50 PST
Comment on attachment 357405 [details]
Patch

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

>> Source/WebKitLegacy/mac/WebCoreSupport/WebAlternativeTextClient.h:35
>> +    explicit WebAlternativeTextClient(WebView *);
> 
> Remove space between "WebView" and "*".

In this case, WebView is an Objective-C class name. Our coding style document says that we should format it this way.

>> Source/WebKitLegacy/mac/WebCoreSupport/WebAlternativeTextClient.h:54
>> +    WebView *m_webView;
> 
> Shouldn't this be "WebView* m_webView;"?  Why move the placement of the space?

Ditto.
Comment 4 Darin Adler 2018-12-15 16:16:29 PST
Hmm, strangely the WebKit coding style guidelines on the website no longer say this. The now say “in non-C++ code” instead of having the rule depend on the type of the pointed-to object. I like the new guideline better, but I don’t remember the WebKit project discussion about making this change.
Comment 5 Darin Adler 2018-12-15 16:21:00 PST
I’ll make the changes you suggested, Mark. Not important either way, and I choose to interpret the wording in the latest draft of the document as a new guideline.
Comment 6 Darin Adler 2018-12-15 16:21:31 PST
Committed r239255: <https://trac.webkit.org/changeset/239255>
Comment 7 Radar WebKit Bug Importer 2018-12-15 16:28:20 PST
<rdar://problem/46757455>
Comment 8 Alexey Proskuryakov 2018-12-17 14:00:15 PST
(In reply to Darin Adler from comment #4)
> Hmm, strangely the WebKit coding style guidelines on the website no longer
> say this. The now say “in non-C++ code” instead of having the rule depend on
> the type of the pointed-to object. I like the new guideline better, but I
> don’t remember the WebKit project discussion about making this change.

Looks like the language in guidelines never changed. The rule for pointers was added in almost the current form in r12863 13 years ago. The only change is that references are no longer mentioned for non-C++ code.

But it is also my memory that we always interpreted this as object type difference, not as file type difference. Given that .m files can become .mm ones, having a fle type difference doesn't make a lot of sense.
Comment 9 Michael Catanzaro 2018-12-17 15:29:39 PST
(In reply to Alexey Proskuryakov from comment #8)
> But it is also my memory that we always interpreted this as object type
> difference, not as file type difference. Given that .m files can become .mm
> ones, having a fle type difference doesn't make a lot of sense.

FWIW it definitely makes sense for C vs. C++ types. E.g. we always write e.g. WebKitWebView* in WebKit C++ code rather than WebKitWebView * as we do in the public headers. Style checker would rightly throw a fit otherwise.

.m vs. .mm may be an entirely different matter and the desirable behavior there might be different, of course.