Bug 73502 – Use const for local variables in KURL::init() and size_t in copyASCII()

• Component: New Bugs
• Status: RESOLVED WONTFIX

[David Kilzer (:ddkilzer)]

Use const pointers in KURL::init() and size_t in copyASCII()

[David Kilzer (:ddkilzer)]

Created attachment 117300 [details]
Patch v1

[WebKit Review Bot]

Attachment 117300 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Why the emphasis on “pointers” here? The const is just a local variable. The fact that it’s a pointer is not all that interesting.

[Antti Koivisto]

Comment on attachment 117300 [details]
Patch v1

r=me

[Antti Koivisto]

The title could be better as it is not pointers only.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> Why the emphasis on “pointers” here? The const is just a local variable. The fact that it’s a pointer is not all that interesting.

(In reply to comment #5)
> The title could be better as it is not pointers only.

Fixed title.  Will post another patch to incorporate David Levin's feedback on webkit-dev.

[David Kilzer (:ddkilzer)]

Created attachment 118482 [details]
Patch

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Created an attachment (id=118482) [details]
> Patch

This addressed the title and ChangeLog wording fixes highlighted by Antti and Darin, and makes baseCharacters a const pointer per email from David Levin on webkit-dev[1].

[1] <message://%3cCACmjMJQzEqCvpODqk--JSmHnX1BEyQDsYMo_=[e-mail]%3e>

[Darin Adler]

Comment on attachment 118482 [details]
Patch

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

> Source/WebCore/platform/KURL.cpp:262
> -static void copyASCII(const UChar* src, int length, char* dest)
> +static void copyASCII(const UChar* src, const size_t length, char* dest)
>  {
> -    for (int i = 0; i < length; i++)
> +    for (size_t i = 0; i < length; i++)
>          dest[i] = static_cast<char>(src[i]);
>  }

Why on earth would we use const here?

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> (From update of attachment 118482 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118482&action=review
> 
> > Source/WebCore/platform/KURL.cpp:262
> > -static void copyASCII(const UChar* src, int length, char* dest)
> > +static void copyASCII(const UChar* src, const size_t length, char* dest)
> >  {
> > -    for (int i = 0; i < length; i++)
> > +    for (size_t i = 0; i < length; i++)
> >          dest[i] = static_cast<char>(src[i]);
> >  }
> 
> Why on earth would we use const here?

Because the length parameter should never change within the method.

Is there a technical reason that it shouldn't be used, or is this more of a style/personal preference issue?

[Darin Adler]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 118482 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=118482&action=review
> > 
> > > Source/WebCore/platform/KURL.cpp:262
> > > -static void copyASCII(const UChar* src, int length, char* dest)
> > > +static void copyASCII(const UChar* src, const size_t length, char* dest)
> > >  {
> > > -    for (int i = 0; i < length; i++)
> > > +    for (size_t i = 0; i < length; i++)
> > >          dest[i] = static_cast<char>(src[i]);
> > >  }
> > 
> > Why on earth would we use const here?
> 
> Because the length parameter should never change within the method.
> 
> Is there a technical reason that it shouldn't be used, or is this more of a style/personal preference issue?

It is a style issue.

Almost all arguments of almost all functions are not changed within the function. For example, this function could be written like this:

    static void copyASCII(const UChar* const src, const size_t length, char* const dest)

This would indicate that src, length, and dest are all unchanged in the implementation of the function. I think that is harder to read than the existing function. And I am not sure why length stands out from the other arguments as worthwhile to call out as unchanged. Does that help people reading the code? It seems unnecessarily cluttered to include const all the time. If const was the default, then I would agree that we should leave all arguments and local variables const except for the ones we plan to modify, but since it’s not the default I prefer that we not put const everywhere.

Stating const when there is something unclear and interesting to be clarified or emphasized is a different story. We’ve been discussing cases like that, for example I think you and Antti have been highlighting a case where there is a local variable in a large complex function and it might be useful to emphasize the point that it is never changed.

For a two line function, declaring the arguments const seems not to clarify sufficiently to be worth the clutter.

I’d like some sort of rule, even if a subjective one, about when we use it. It doesn’t have to be a firm machine-enforceable rule, but I do want consistency and not decisions made differently based on different programmers’ whims. I find it easier to work on WebKit because we use on a consistent basic coding style and adhere to it most of the time.

The rule I suggested until now was “don’t use const for the values of arguments or of local variables” -- we’d still use const for members and of for the types the references refer to and pointers point to -- but I believe you wrote this bug at least in part because you are dissatisfied with that rule. So I’d like to make sure we have a new one.

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> (In reply to comment #10)
> > Is there a technical reason that it shouldn't be used, or is this more of a style/personal preference issue?
> 
> It is a style issue.
> 
> Almost all arguments of almost all functions are not changed within the function. For example, this function could be written like this:
> 
>     static void copyASCII(const UChar* const src, const size_t length, char* const dest)

Ahh!  I agree that would detract the readability of the code.

> I’d like some sort of rule, even if a subjective one, about when we use it. It doesn’t have to be a firm machine-enforceable rule, but I do want consistency and not decisions made differently based on different programmers’ whims. I find it easier to work on WebKit because we use on a consistent basic coding style and adhere to it most of the time.
> 
> The rule I suggested until now was “don’t use const for the values of arguments or of local variables” -- we’d still use const for members and of for the types the references refer to and pointers point to -- but I believe you wrote this bug at least in part because you are dissatisfied with that rule. So I’d like to make sure we have a new one.

Another way to look at this is that if KURL::init() were split up into smaller functions/methods, the need for making local variables const would greatly decrease since one could read the entire block of code to see how all of the variables are used (and thus which ones should not be changed because they're used for calculating lengths).

Or to put it yet another way, my desire to add const to local variables is a really a code smell that probably means the method is too large to easily understand/read.

However, refactoring KURL::init() is not at the top of my priority list, so I'm just going to close this bug as RESOLVED/WONTFIX.

Thanks for the explanation.

[David Kilzer (:ddkilzer)]

Comment on attachment 118482 [details]
Patch

Clearing flags and marking obsolete.

[Ryosuke Niwa]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 118482 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=118482&action=review
> > 
> > > Source/WebCore/platform/KURL.cpp:262
> > > -static void copyASCII(const UChar* src, int length, char* dest)
> > > +static void copyASCII(const UChar* src, const size_t length, char* dest)
> > >  {
> > > -    for (int i = 0; i < length; i++)
> > > +    for (size_t i = 0; i < length; i++)
> > >          dest[i] = static_cast<char>(src[i]);
> > >  }
> > 
> > Why on earth would we use const here?
> 
> Because the length parameter should never change within the method.
> 
> Is there a technical reason that it shouldn't be used, or is this more of a style/personal preference issue?

This is a clear cut case where the constness of a variable is self-evident (the function is only 2 lines!), and there's almost no benefit in explicitly marking it as const.