Bug 34605 - check-webkit-style incorrectly complains about NULL appearing in a comment
Summary: check-webkit-style incorrectly complains about NULL appearing in a comment
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-04 10:57 PST by Gustavo Noronha (kov)
Modified: 2010-07-20 00:17 PDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2010-02-04 10:57:17 PST
See https://bugs.webkit.org/show_bug.cgi?id=34602

check-webkit-style reported this:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/soup/DNSSoup.cpp:41:  Use 0 instead of NULL. 
[readability/null] [4]
Total errors found: 1

And this is the line it's complaining about:

 41     // We may get invalid hostnames, so NULL-check here.
Comment 1 David Levin 2010-07-19 23:30:47 PDT
(In reply to comment #0)
> See https://bugs.webkit.org/show_bug.cgi?id=34602
> 
> check-webkit-style reported this:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebCore/platform/network/soup/DNSSoup.cpp:41:  Use 0 instead of NULL. 
> [readability/null] [4]
> Total errors found: 1
> 
> And this is the line it's complaining about:
> 
>  41     // We may get invalid hostnames, so NULL-check here.

fwiw, this check was done this way on purpose. The check was carefully done to include both code and comments but not items in quotes (in code).

 Several experienced reviews have commented on NULL appearing in comments. As I understand it, the reason being that NULL isn't allowed in code so it doesn't make sense in comments. 

For example, the comment could easily have said
  // Check for invalid hostnames.
and seems to talk more about why than what (checking for a 0 value).

Another recent instance is this comment 
  if (hdc) { // Can this ever actually be NULL?
which could have easily been 
  if (hdc) { // Can hdc ever actually be 0?
Comment 2 Alexey Proskuryakov 2010-07-20 00:17:42 PDT
Yes, I agree that this particular comment needn't mention null check at all. And we do prefer to talk about null or 0 when necessary.